Rename '[in-process|remote]-browser-[frame-]shown' observer notifications and send when all frames are created

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

Trunk
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

TabParent.cpp already sends a notification 'oop-frameloader-crashed' when a remote frame crashes.  It would be useful for getting mochitests running with e10s enabled for a notification to be sent as the browser is created.  Specifically, and possibly only in the short-medium term, test code will observe this and inject some test specific code into each browser as it is created.
Attachment #828366 - Flags: feedback?(bugs)
Blocks: 935799
Comment on attachment 828366 [details] [diff] [review]
Add oop-frameloader-created notification as frame is created.

If testing is the only user, couldn't you use MutationObserver to check
when a new <browser remote="true"> is created?

Or could we send remote-browser-frame-shown for all the remote browsers?
Attachment #828366 - Flags: feedback?(bugs)
(In reply to Olli Pettay [:smaug] from comment #1)
> Comment on attachment 828366 [details] [diff] [review]
> Add oop-frameloader-created notification as frame is created.
> 
> If testing is the only user, couldn't you use MutationObserver to check
> when a new <browser remote="true"> is created?

In my experiments, it seems that fires too late for my needs - by the time the mutation observer is called the test script has already started running.  I'm not surprised by this, but let me know if you are - it's always possible I did something wrong.

> Or could we send remote-browser-frame-shown for all the remote browsers?

Ahh, yeah, that works fine if I make the following change to content/base/src/nsFrameLoader.cpp:

-    if (OwnerIsBrowserOrAppFrame() && os && !mRemoteBrowserInitialized) {
+    if (os && !mRemoteBrowserInitialized) {

which I assume is what you meant.  There are a few other observers in the tree of this notification, and it's not clear to me if some extra action needs to be taken for them - they *look* OK to me, but that's only because I don't really know what I'm looking at :)

Would a patch like that be acceptable, and if so, do you believe changes to existing observers would be necessary?

Thanks!
Flags: needinfo?(bugs)
You should probably add checks to existing notification users that frameloader's owner is browser or app.
Flags: needinfo?(bugs)
renaming bug to reflect the new approach.
Summary: Send 'oop-frameloader-created' observer notification when a remote frame is created → Send 'remote-browser-frame-shown' observer notification when all remote frames are created
A problem with having other observers check reallyIsBrowserOrApp is that the HTMLIFrameElement element doesn't expose this method, nor a way to get the nsIMozBrowserFrame from JS code.  bz helped me work out how to add this to HTMLIFrameElement, which this patch does.  Another alternative would have been to add it directly to nsIFrameLoader or to pass something in |data| to reflect this, but adding it to HTMLIFrameElement seems the best option.
Assignee: nobody → mhammond
Attachment #832061 - Flags: feedback?(bugs)
This patch removes the check for reallyIsBrowserOrApp from when the observer notification is sent, and adds the check to all existing observers of the event.  It relies on the reallyIsBrowserOrApp attribute added in part 1.
Attachment #828366 - Attachment is obsolete: true
Attachment #832062 - Flags: feedback?(bugs)
Polite feedback ping for smaug
Attachment #832061 - Flags: feedback?(bugs) → feedback+
Comment on attachment 832062 [details] [diff] [review]
Unconditionally send the notification and have existing observers check if reallyIsBrowserOrApp

Notification aren't still quite perfect since
in-process-browser-or-app-frame-shown is clearly for browser-or-app.

Could we change remote-browser-frame-shown to 
remote-browser-shown
Attachment #832062 - Flags: feedback?(bugs) → feedback+
As per discussion on IRC, we will rename 'remote-browser-frame-shown' to 'remote-browser-shown' and 'in-process-browser-or-app-frame-shown' to 'inprocess-browser-shown' and send them both unconditionally.  All existing consumers of these notifications will be updated to the new names and have their own check for reallyIsBrowserOrApp.
Summary: Send 'remote-browser-frame-shown' observer notification when all remote frames are created → Rename '[in-process|remote]-browser-[frame-]shown' observer notifications and send when all frames are created
Attachment #832061 - Flags: review?(bugs)
This patch renames 'remote-browser-frame-shown' to
'remote-browser-shown' and 'in-process-browser-or-app-frame-shown' to
'inprocess-browser-shown' and sends them both unconditionally.  All existing
consumers of these notifications have been updated to the new names and have
their own check for reallyIsBrowserOrApp.
Attachment #832062 - Attachment is obsolete: true
Attachment #8340170 - Flags: review?(bugs)
Attachment #832061 - Flags: review?(bugs) → review+
Attachment #8340170 - Flags: review?(bugs) → review+
This has turned into a rabbit warren.  The original patches here failed on try.  With bz's help, I tracked this down to the fact that as soon as the JS code referenced .ownerElement, XBL bindings may have been created.  This caused one <editor> test to fail on try, with the editor complaining it had a null docShell.  bz suggested I look at changing nsContainerBoxObject's docView getter to use nsIFrameLoaderOwner to return the docShell, but various attempts at this failed on try with even more obscure errors, including an assertion about mis-matched JS compartments.

At this point I gave up and decided to take a much easier route.  These new patches add an ownerIsBrowserOrAppFrame xpcom getter to nsIFrameLoader instead of the getter on the HTMLIFrame.  This means the javascript code can perform the check without referencing ownerElement, and thus avoid the premature creation of the XBL bindings.

green try with this new approach at https://tbpl.mozilla.org/?tree=Try&rev=fabb1276044e

Sorry smaug, but this means a new review.  I should also note that an alternative approach would have been to send the result of OwnerIsBrowserOrAppFrame() in the notification's |data|, but that felt a little less clean - but it would avoid the need for the new xpcom attribute.
Attachment #832061 - Attachment is obsolete: true
Attachment #8343462 - Flags: review?(bugs)
This patch is almost identical to the last round, but instead of checking frameLoader.ownerElement.reallyIsBrowserOrApp, it checks frameLoader.ownerIsBrowserOrAppFrame
Attachment #8340170 - Attachment is obsolete: true
Attachment #8343463 - Flags: review?(bugs)
Attachment #8343462 - Flags: review?(bugs) → review+
Attachment #8343463 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/65061e971eb8
https://hg.mozilla.org/mozilla-central/rev/a7fc463961ce
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Depends on: 950441
This broke our travis tests. Can we back out?
Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e2de961acc

When this relands, can we please try to include a test that would hopefully catch bug 950441 on TBPL?
Status: RESOLVED → REOPENED
Flags: in-testsuite?
Resolution: FIXED → ---
Target Milestone: mozilla29 → ---
Ryan> will likely be difficult to catch. I mean, without running the marionette JS tests on TBPL ;)
Depends on: 952176
Status: REOPENED → NEW
This is an updated version of a patch that was reviewed a few months ago, but in those few months things have changed enough that a new review is prudent.

This old patch renamed remote-browser-frame-shown -> remote-browser-shown and in-process-browser-or-app-frame-shown -> inprocess-browser-shown.  This new patch also renames remote-browser-frame-pending -> remote-browser-pending, and like the other notifications, unconditionally sends it and relies on the observer to check reallyIsBrowserOrAppFrame.  Also, b2g/chrome/content/devtools.js now handles these notifications so this is updated to the new world order.

Note I did a search of gaia on github and it doesn't use remote-browser-frame-pending, so hopefully landing this isn't going to strike the same problem we had before.

Try at https://tbpl.mozilla.org/?tree=Try&rev=84f1a997db7f
Attachment #8343463 - Attachment is obsolete: true
Attachment #8378805 - Flags: review?(bugs)
Attachment #8378805 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/4b65aed6410f
https://hg.mozilla.org/mozilla-central/rev/cdcdbc0df8bf
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
While fixing bug 1044736 I found remote-browser-frame-shown is not fixed in BrowserElementParent.jsm, I wonder why our automation didn't catch this.
You need to log in before you can comment on or make changes to this bug.