898170 - Minor tweaks to get electrolysis working on trunk again

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We've mostly been focusing on the larch branch, but now we'd like to port some changes back to trunk to make it more usable. I just have a few small patches to post in this bug.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: don't try to swap docshells

We're not able to swap docshells with remote tabs, so this disables it.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: set initial content title to empty string

In some cases we try to read the content title before it's updated by the child process. The caller doesn't expect it to be null. This patch just makes the default be the empty string.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: don't remove listeners from XBL destructor

When we tear down a remote <browser> element, this is what happens:

1. element is removed from the document
2. it therefore loses all of its special properties, like the frameloader
3. the XBL destructor runs, and tries to remove message listeners
4. in doing so, it tries to get the message manager, which is obtained via this getter:

      <property name="messageManager"
                onget="return this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.messageManager;"
                readonly="true"/>

5. we throw because frameLoader is null

This patch takes out the code that removes the listeners. They'll already have been removed anyway when the browser element is removed from the document.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: WP.isToplevel should revert to true after calling listeners

This problem arises because sometimes chrome code will call its own web progress listeners and pass them the top-level nsIWebProgress object. We only set the isToplevel property of these objects right before calling their listeners. So this could happen:

1. RemoteWebProgress.jsm calls the WP listeners with isToplevel = false.
2. Chrome code decides to call the listeners itself. Now they get invoked with isToplevel = false, which is wrong.

This patch just sets isToplevel back to true after running all the listeners.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 781277 [details] [diff] [review]
don't remove listeners from XBL destructor

Review of attachment 781277 [details] [diff] [review]:
-----------------------------------------------------------------

This is slightly odd, I don't think the frameLoader should be gone before the XBL destructor runs..  But XBL destruction order is AFAIK chaotic..

I guess this is fine for now, and hopefully not removing the listeners don't make it leak, but it'd be nice if someone like Smaug could take a look at these two concerns to clarify, because the code being removed here doesn't strike me as "wrong"

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: disable password manager content scripts

It looks like the password manager code is moving closer to using content scripts and being e10s-friendly, which is awesome. However, it's not quite there yet. It calls into nsLoginManager.js, which needs to talk to a central password store.

This patch just skips the listener stuff in the content script when we're using electrolysis.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 781288 [details] [diff] [review]
disable password manager content scripts

Review of attachment 781288 [details] [diff] [review]:
-----------------------------------------------------------------

can you just have a single if() that wraps all of the addEventListeners instead of doing it inside the event handler?

Do you recall what breaks without this? does it throw an expection and that makes the events not delivered to other listeners?

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 781283 [details] [diff] [review]
WP.isToplevel should revert to true after calling listeners

Review of attachment 781283 [details] [diff] [review]:
-----------------------------------------------------------------

oh well, http://i.imgur.com/VAAV5Ie.jpg

hopefully we'll move away from this interface anyways over time, and it will be left only as a compat shim for add-ons. And this patch brings good benefits for proper dogfooding e10s, so let's do it

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: WP.isTopLevel alternative

Although, what do you think of this alternative? Same code, just handled in a different place. Cleaner or worse?

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to :Felipe Gomes from comment #9)
> Created attachment 781490 [details] [diff] [review]
> WP.isTopLevel alternative
> 
> Although, what do you think of this alternative? Same code, just handled in
> a different place. Cleaner or worse?

That seems fine to me, although I don't like the idea of a property that's only sometimes defined. Could we just set it to undefined or null at other times?

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: disable password manager content scripts, v2

This puts the if statement around the whole addListener block, which is much better.

This is the error I got:

System JS : ERROR file:///home/billm/mozilla/in1/objdir-ff-dbg/dist/bin/components/nsLoginManager.js:353
                     TypeError: this._storage is null
System JS : ERROR resource://gre/modules/LoginManagerContent.jsm:500
                     NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "this._storage is null" {file: "file:///home/billm/mozilla/in1/objdir-ff-dbg/dist/bin/components/nsLoginManager.js" line: 353}]' when calling method: [nsILoginManager::countLogins]

I don't think that it actually made anything stop functioning. I just don't like the idea of calling code that doesn't work and that reports errors. Having lots of error reports makes it easy to miss important errors.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Attachment: isTopLevel

Yeah, null sounds better

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 781849 [details] [diff] [review]
isTopLevel

Review of attachment 781849 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a peer, but I don't think anyone will mind in this case.

::: toolkit/modules/RemoteWebProgress.jsm
@@ +68,5 @@
> +    // through browser.webProgress and thus represents the top-level
> +    // document.
> +    // However, during message handling it temporarily represents
> +    // the webProgress that generated the notification, which may or
> +    // may not be a non-toplevel frame.

Change non-toplevel to toplevel?

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1fb07a6886de
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bf57ad8c6635
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/676ae4615bda
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3aef6df15454
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/be0befd009cb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1fb07a6886de
https://hg.mozilla.org/mozilla-central/rev/bf57ad8c6635
https://hg.mozilla.org/mozilla-central/rev/676ae4615bda
https://hg.mozilla.org/mozilla-central/rev/3aef6df15454
https://hg.mozilla.org/mozilla-central/rev/be0befd009cb