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

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

Attachment: Add oop-frameloader-created notification as frame is created.

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(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!

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

You should probably add checks to existing notification users that frameloader's owner is browser or app.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

renaming bug to reflect the new approach.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-935793-part-1-Add-reallyIsBrowserOrApp-to-HTMLIF.patch

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.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: Unconditionally send the notification and have existing observers check if reallyIsBrowserOrApp

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.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Try at https://tbpl.mozilla.org/?tree=Try&rev=e04cca282edf

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Polite feedback ping for smaug

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 10

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-935793-part-2-rename-browser-shown-notifications.patch

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.

Comment 12

[Mark Hammond [:markh] [:mhammond]]

https://tbpl.mozilla.org/?tree=Try&rev=279a9d4dcaa2

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-935793-part-1-Add-ownerIsBrowserOrApp-to-nsIFram.patch

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.

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-935793-part-2-rename-browser-shown-notifications.patch

This patch is almost identical to the last round, but instead of checking frameLoader.ownerElement.reallyIsBrowserOrApp, it checks frameLoader.ownerIsBrowserOrAppFrame

Comment 15

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/65061e971eb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7fc463961ce

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65061e971eb8
https://hg.mozilla.org/mozilla-central/rev/a7fc463961ce

Comment 17

[Gregor Wagner [:gwagner]]

This broke our travis tests. Can we back out?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/a4e2de961acc

Comment 20

[Julien Wajsberg [:julienw]]

Ryan> will likely be difficult to catch. I mean, without running the marionette JS tests on TBPL ;)

Comment 21

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-935793-part-2-rename-browser-shown-notifications.patch

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

Comment 22

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b65aed6410f
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdcdbc0df8bf

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b65aed6410f
https://hg.mozilla.org/mozilla-central/rev/cdcdbc0df8bf

Comment 24

[Kan-Ru Chen [:kanru] (UTC+9)]

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.