Closed Bug 898170 Opened 6 years ago Closed 6 years ago

Minor tweaks to get electrolysis working on trunk again

Categories

(Firefox :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25

People

(Reporter: billm, Assigned: billm)

Details

Attachments

(6 files, 2 obsolete files)

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.
We're not able to swap docshells with remote tabs, so this disables it.
Attachment #781272 - Flags: review?(felipc)
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.
Attachment #781274 - Flags: review?(felipc)
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.
Attachment #781277 - Flags: review?(felipc)
Attachment #781272 - Flags: review?(felipc) → review+
Attachment #781274 - Flags: review?(felipc) → review+
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.
Attachment #781283 - Flags: review?(felipc)
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"
Attachment #781277 - Flags: review?(felipc) → review+
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.
Attachment #781288 - Flags: review?(felipc)
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?
Attachment #781288 - Flags: review?(felipc)
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
Attachment #781283 - Flags: review?(felipc) → review+
Attached patch WP.isTopLevel alternative (obsolete) — Splinter Review
Although, what do you think of this alternative? Same code, just handled in a different place. Cleaner or worse?
Attachment #781490 - Attachment is patch: true
Attachment #781490 - Attachment mime type: text/x-patch → text/plain
(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?
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.
Attachment #781288 - Attachment is obsolete: true
Attachment #781829 - Flags: review?(felipc)
Attachment #781829 - Attachment is patch: true
Attachment #781829 - Flags: review?(felipc) → review+
Attached patch isTopLevelSplinter Review
Yeah, null sounds better
Attachment #781490 - Attachment is obsolete: true
Attachment #781849 - Flags: review?(wmccloskey)
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?
Attachment #781849 - Flags: review?(wmccloskey) → review+
You need to log in before you can comment on or make changes to this bug.