Closed Bug 736688 Opened 8 years ago Closed 8 years ago

Re-implement <iframe mozbrowser> in JS

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

(Blocks 2 open bugs)

Details

Attachments

(10 files, 2 obsolete files)

9.55 KB, patch
Details | Diff | Splinter Review
6.17 KB, patch
Details | Diff | Splinter Review
9.44 KB, patch
Details | Diff | Splinter Review
16.22 KB, patch
Details | Diff | Splinter Review
11.83 KB, patch
Details | Diff | Splinter Review
2.99 KB, patch
Details | Diff | Splinter Review
10.49 KB, patch
Details | Diff | Splinter Review
3.62 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.91 KB, patch
smaug
: review+
Details | Diff | Splinter Review
82.50 KB, patch
Details | Diff | Splinter Review
Attached patch WIP v1 (see BrowserAPI.js) (obsolete) — 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: nobody → justin.lebar+bug
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.
One advantage is, if it's in JS, you can copy the existing browser front-end code (for Firefox and Fennec).
\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.
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?
(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.
(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.
Attachment #606825 - Attachment is obsolete: true
New try push: https://tbpl.mozilla.org/?tree=Try&rev=400161aa6be2

Smaug, these changes are mostly mechanical, except parts 6 and 7.
At smaug's request...
Attachment #607616 - Flags: review?(bugs)
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.
Aha, this is why the tests were failing on try.
Attachment #607842 - Flags: review?(bugs)
Blocks: 738172
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 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 on attachment 607544 [details] [diff] [review]
Part 1: Fix up browser frame tests.

I'm reviewing the full patch.
Attachment #607544 - Flags: review?(bugs)
Attachment #607545 - Flags: review?(bugs)
Attachment #607546 - Flags: review?(bugs)
Attachment #607547 - Flags: review?(bugs)
Attachment #607548 - Flags: review?(bugs)
Attachment #607549 - Flags: review?(bugs)
Attachment #607550 - Flags: review?(bugs)
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-
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.
> Maybe (hopefully this works)
> var evt = new Event('mozbrowser' + name);

We should mark these events as trusted, but I'm not sure how.
(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.
Attachment #609814 - Flags: review?(bugs)
Attachment #607616 - Attachment is obsolete: true
Note to self: Rename BrowserAPI.js to BrowserFrameAPI.js.
Comment on attachment 609814 [details] [diff] [review]
Part 9: Review comments

Rename BrowserAPI as discussed on IRC
Attachment #609814 - Flags: review?(bugs) → review+
Sorry about my horrible review :(
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?
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.
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.
Sure, iterative landings are encouraged an reduce engineering risk. We should definitely take this and file a follow-up blocker bug.
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.
> looks like missing some parts?

No, I just rejiggered the patches before landing.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Depends on: 740479
Depends on: 740481
Depends on: 742448
No longer blocks: 740836
You need to log in before you can comment on or make changes to this bug.