Closed
Bug 736688
Opened 13 years ago
Closed 13 years ago
Re-implement <iframe mozbrowser> in JS
Categories
(Firefox OS Graveyard :: General, defect)
Firefox OS Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
References
Details
Attachments
(10 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
I'd like to re-implement <iframe mozbrowser> in JS. This should make some things, such as overriding methods, much easier. It will also, I hope, allow JS hackers (coughbenfrancis) to modify the API themselves. WIP attached. Look at BrowserAPI.js, if you're curious. It currently works for locationchange, loadstart, and loadend. I haven't done titlechange yet, and top/parent/frameElement is not working for reasons unclear at the moment.
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → justin.lebar+bug
Comment 1•13 years ago
|
||
As a Front End Developer and not a Gecko hacker, I have no idea whether this is a good idea or not! Whether written in C++ or JavaScript, this stuff is all new to me. Sure C++ is a barrier to me hacking on API implementations myself (though I have used C before), but not as much as understanding the architecture of how all this stuff is implemented, particularly the Docshell. I'm happy to try to learn how this stuff works if it's necessary for me to get my work done.
Assignee | ||
Comment 2•13 years ago
|
||
One advantage is, if it's in JS, you can copy the existing browser front-end code (for Firefox and Fennec).
Comment 3•13 years ago
|
||
\o/ Justin, thanks for doing this. This will indeed help us a lot if we need to somewhat port the code from fennec that sends messages to the Java UI.
Comment 5•13 years ago
|
||
The next most pressing requirement for the Browser API actually seems to be to allow content full access to browsing history. Will this change make that easier at all?
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Ben Francis from comment #5) > The next most pressing requirement for the Browser API actually seems to be > to allow content full access to browsing history. Will this change make that > easier at all? Yes, I'll step you through that. Shouldn't be too hard.
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #4) > https://tbpl.mozilla.org/?tree=Try&rev=d3fda4ee0a48 The browser frame test suite failed in this push, but I think it's because I pushed atop a bad revision. I'll push again, but I'm also going to request review on what I have.
Assignee | ||
Updated•13 years ago
|
Attachment #606825 -
Attachment is obsolete: true
Assignee | ||
Comment 15•13 years ago
|
||
New try push: https://tbpl.mozilla.org/?tree=Try&rev=400161aa6be2 Smaug, these changes are mostly mechanical, except parts 6 and 7.
Assignee | ||
Comment 17•13 years ago
|
||
This is still permaorange on try, but the tests pass just fine on my machine. It's as though the mozbrowser isn't being initialized, somehow. I'm not sure what's going on.
Assignee | ||
Comment 18•13 years ago
|
||
Aha, this is why the tests were failing on try.
Attachment #607842 -
Flags: review?(bugs)
Assignee | ||
Updated•13 years ago
|
Blocks: browser-api
Comment 19•13 years ago
|
||
Comment on attachment 607842 [details] [diff] [review] Part 8: Add BrowserAPI.{js,manifest} to package manifests. Horrible to copy the same thing in many places.
Attachment #607842 -
Flags: review?(bugs) → review+
Comment 20•13 years ago
|
||
Comment on attachment 607616 [details] [diff] [review] Roll-up patch v1, parts 1-7 combined >@@ -371,223 +289,10 @@ nsGenericHTMLFrameElement::GetReallyIsBrowser() > principal->GetURI(getter_AddRefs(principalURI)); > if (!nsContentUtils::URIIsChromeOrInPref(principalURI, > "dom.mozBrowserFramesWhitelist")) { >- return false; >+ return NS_OK; > } > > // Otherwise, succeed. >+ *aOut = true; > return true; > } So I doubt you want the method to return nsresults and bools. >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp >index 645a77a..32e6bce 100644 >--- a/docshell/base/nsDocShell.cpp >+++ b/docshell/base/nsDocShell.cpp >@@ -11768,6 +11768,42 @@ nsDocShell::GetIsBrowserFrame(bool *aOut) > NS_IMETHODIMP > nsDocShell::SetIsBrowserFrame(bool aValue) > { >+ // Disallow transitions from browser frame to not-browser-frame. Once a >+ // browser frame, always a browser frame. (Othwerise, observers of Otherwise >+ // docshell-marked-as-browser-frame would have to distinguish between >+ // newly-created browser frames and frames which went from true to false back >+ // to true.) >+ MOZ_ASSERT_IF(mIsBrowserFrame, aValue); This should throw, not crash if mIsBrowserFrame is already set. >+ >+ var evt = win.document.createEvent('event'); >+ evt.initEvent('mozbrowser' + name, >+ /* bubbles = */ false, >+ /* cancelable = */ false); Maybe (hopefully this works) var evt = new Event('mozbrowser' + name); >+ var evt = win.document.createEvent('customevent'); >+ evt.initCustomEvent('mozbrowser' + name, >+ /* bubbles = */ false, >+ /* cancelable = */ false, >+ data); var evt = new win.CustomEvent('mozbrowser' + name, { detail: data }); Someone more familiar with js components should also review the new BrowserAPI.js. And, I think the name should be something different. BrowserAPI is just too generic. (re-reading the js parts...)
Comment 21•13 years ago
|
||
Comment on attachment 607544 [details] [diff] [review] Part 1: Fix up browser frame tests. I'm reviewing the full patch.
Attachment #607544 -
Flags: review?(bugs)
Updated•13 years ago
|
Attachment #607545 -
Flags: review?(bugs)
Updated•13 years ago
|
Attachment #607546 -
Flags: review?(bugs)
Updated•13 years ago
|
Attachment #607547 -
Flags: review?(bugs)
Updated•13 years ago
|
Attachment #607548 -
Flags: review?(bugs)
Updated•13 years ago
|
Attachment #607549 -
Flags: review?(bugs)
Updated•13 years ago
|
Attachment #607550 -
Flags: review?(bugs)
Comment 22•13 years ago
|
||
Comment on attachment 607616 [details] [diff] [review] Roll-up patch v1, parts 1-7 combined >+++ b/dom/base/BrowserAPI.js >@@ -0,0 +1,258 @@ >+/* This Source Code Form is subject to the terms of the Mozilla Public >+ * License, v. 2.0. If a copy of the MPL was not distributed with this file, >+ * You can obtain one at http://mozilla.org/MPL/2.0/. */ >+ >+"use strict"; >+ >+let Cu = Components.utils; >+let Ci = Components.interfaces; >+let Cc = Components.classes; >+Cu.import("resource://gre/modules/XPCOMUtils.jsm"); >+ >+/** >+ * The BrowserAPI implements <iframe mozbrowser>. >+ * >+ * We detect windows and docshells contained inside <iframe mozbrowser>s and >+ * alter their behavior so that the page inside the iframe can't tell that it's >+ * framed and the page outside the iframe can observe changes within the iframe >+ * (e.g. loadstart/loadstart, locationchange). >+ */ >+ >+function BrowserAPI() {} >+BrowserAPI.prototype = { >+ classID: Components.ID("{5d6fcab3-6c12-4db6-80fb-352df7a41602}"), >+ QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, >+ Ci.nsISupportsWeakReference]), >+ >+ /** >+ * The keys of this map are the set of chrome event handlers we've observed >+ * which contain a mozbrowser window. >+ * >+ * The values in this map are ignored. >+ */ >+ _chromeEventHandlersWatching: new WeakMap(), >+ >+ /** >+ * The keys of this map are the set of windows we've observed that are >+ * directly contained in <iframe mozbrowser>s. >+ * >+ * The values in this map are ignored. >+ */ >+ _topLevelBrowserWindows: new WeakMap(), >+ >+ _init: function BA_init() { >+ this._progressListener._browserAPI = this; >+ >+ var os = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); >+ os.addObserver(this, 'content-document-global-created', /* ownsWeak = */ true); >+ os.addObserver(this, 'docshell-marked-as-browser-frame', /* ownsWeak = */ true); I think I'd prefer having just one notification which globalwindow fires when global for mozbrowser is created. It would work like content-document-global-created, but would not fire for all the windows >+ _observeDocshellMarkedAsBrowserFrame: function BA_observeDocshellMarkedAsBrowserFrame(docshell) { >+ docshell.QueryInterface(Ci.nsIWebProgress) >+ .addProgressListener(this._progressListener, >+ Ci.nsIWebProgress.NOTIFY_LOCATION | >+ Ci.nsIWebProgress.NOTIFY_STATE_WINDOW); >+ }, ... and in that case there should be perhaps a weakmap for docshells to which addProgressListener has been added. I'd like to see a new patch.
Attachment #607616 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 23•13 years ago
|
||
I think I remember now why I added the new docshell notification. Without it, on the initial load into an <iframe mozbrowser>, we wouldn't get a loadstart notification. The global window gets created *after* the docshell fires that notification. I can probably work around this in the JS, but it won't be pretty. Also, as a note to self, browserapi.js should not register any observers at all if mozbrowser is globally disabled.
Assignee | ||
Comment 24•13 years ago
|
||
> Maybe (hopefully this works)
> var evt = new Event('mozbrowser' + name);
We should mark these events as trusted, but I'm not sure how.
Assignee | ||
Comment 25•13 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #24) > > Maybe (hopefully this works) > > var evt = new Event('mozbrowser' + name); > > We should mark these events as trusted, but I'm not sure how. Okay, that happens automagically because I'm firing from chrome.
Comment 29•13 years ago
|
||
Comment on attachment 609814 [details] [diff] [review] Part 9: Review comments Rename BrowserAPI as discussed on IRC
Attachment #609814 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 31•13 years ago
|
||
I'm tempted to land this as-is, with the vulnerabilities we know about (which I won't list here), since we need to push forward on this API, and bulletproof security is not a priority at this point for B2G. Boris, do you think that's reasonable?
Comment 32•13 years ago
|
||
I severely disagree with comment 31. Not opposed to iterative landing but we are building a product and we need a watertight security story. Security is very much a top priority right now.
Assignee | ||
Comment 33•13 years ago
|
||
Sorry, perhaps I wasn't clear. What I meant was, I thought it would be acceptable to check in insecure code for the sake of having something to improve upon. If we wait until the security story is sorted out, then browser API development may be stalled for weeks. I don't mean to imply that security isn't important, just that it might be acceptable to land this as-is.
Comment 34•13 years ago
|
||
Sure, iterative landings are encouraged an reduce engineering risk. We should definitely take this and file a follow-up blocker bug.
Comment 35•13 years ago
|
||
I'm fine with the plan in comment 31, for what it's worth. Especially since we now have a plan for nuking Components in content, so you won't have to worry about that soon, I hope.
What are the vulnerabilities? We can follow-up privately if need be. Remember that folks will be using this on real phones.
Comment 37•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8e1f2165d52e https://hg.mozilla.org/mozilla-central/rev/e69f1c7562aa https://hg.mozilla.org/mozilla-central/rev/3e8f54937d3b https://hg.mozilla.org/mozilla-central/rev/935db09a4365 https://hg.mozilla.org/mozilla-central/rev/e530ae7f6b74 https://hg.mozilla.org/mozilla-central/rev/199383e4fd2f looks like missing some parts?
Assignee | ||
Comment 38•13 years ago
|
||
> looks like missing some parts?
No, I just rejiggered the patches before landing.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•