Closed Bug 962767 Opened 6 years ago Closed 6 years ago
Docshell swapping gets message manager parent wrong
You can Cmd-Click the back arrow to duplicate the tab and go back to the previous session history entry. This is broken and throws an error: TypeError: docShell is null Stack trace: SessionHistoryInternal.collect@resource://app/modules/sessionstore/SessionHistory.jsm:48 this.SessionHistory<.collect@resource://app/modules/sessionstore/SessionHistory.jsm:28 SyncHandler.collectSessionHistory@chrome://browser/content/content-sessionStore.js:200 TabStateInternal._collectSyncUncached@resource://app/modules/sessionstore/TabState.jsm:329 TabStateInternal.clone@resource://app/modules/sessionstore/TabState.jsm:293 this.TabState<.clone@resource://app/modules/sessionstore/TabState.jsm:61 ssi_duplicateTab@resource:///modules/sessionstore/SessionStore.jsm:1667 ss_duplicateTab@resource:///modules/sessionstore/SessionStore.jsm:208 duplicateTabIn@chrome://browser/content/browser.js:15990 BrowserBack@chrome://browser/content/browser.js:10553 oncommand@chrome://browser/content/browser.xul:1 clickHandler@chrome://browser/content/browser.js:9325 Didn't check but I assume this is caused by bug 942374.
Maybe also bug 961861?
I see the same error when opening and closing some blank tabs using Cmd+T/W.
I'm not seeing this on tip. It sorta sounds like bug 961861, but I don't see why that problem would affect a non-e10s browser. How often does this reproduce for you?
Comment #0 always. Comment #2 in 90% of the cases when rather quickly opening and closing a tab. Tested on fx-team tip. Nightly is affected as well.
Now even comment #0 isn't happening anymore, weird. I didn't even close the browser in the meantime.
OK, I can get the exception by opening and closing tabs a lot.
Ugh, this is a bug in docshell swapping. I wrote a testcase that illustrates the problem. When we swap docshells, something really complicated happens with the message managers. http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1280 My understanding of docshell swapping is that we fix up any pointers that point *into* the docshell/frameloader, but we leave the state that's internal to the docshell/frameloader alone. For some reason that's not happening for the message manager. We actually swap the message managers. We also swap the parent fields of the message managers. To illustrate the problem, consider this scenario: window1: its message manager (MMw1) has listeners [Lw1] it has <browser> element B1 containing frameloader FL1: FL1 has message manager MMf1 with listeners [Lf1] and parent MMw1 window2: its message manager (MMw2) has listeners [Lw2] it has <browser> element B2 containing frameloader FL2: FL2 has message manager MMf2 with listeners [Lf2] and parent MMw2 Now we do a swap. Here is the final state: window1: its message manager (MMw1) has listeners [Lw1] it has <browser> element B1 containing frameloader FL2: FL2 has message manager MMf1 with listeners [Lf1] and parent MMw2 window2: its message manager (MMw2) has listeners [Lw2] it has <browser> element B2 containing frameloader FL2: FL2 has message manager MMf2 with listeners [Lf2] and parent MMw1 The parent message managers are almost definitely wrong. If you attach a listener to a given window, then it should fire on messages from any browser element in that window, even if the browser started out in a different window. I'm not really sure about the message managers or listeners though. Maybe Tim or Gavin have an opinion. Is there a reason things work this way Olli, or is it just a mistake? I think the main conclusion to draw from this bug is that docshell swapping totally sucks. Is there any way we can get rid of it? Moving tabs between windows seems like the hard case.
the parent mm certainly looks like a mistake. Also, note, in the case of in-process-mm frameloader needs to deal with the parent side of the frame message manager and child side of the message manager. The idea was that only the child side would swap, since that is technically bound to the docshell, and the parent side is bound to the frameloader.
er, wait... /me reads some code :)
Yes, so I shouldn't have said bound to frameloader, but parent side of mm should be bound to the window level mm. So changing parentManager is wrong.
I guess tabs are moved from one window to another rather rarely so the wrong parent hasn't showed up in bug reports. That code has been there since spring 2010.
I think this does what you want.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8364162 - Flags: review?(bugs)
6 years ago
Component: Session Restore → DOM: Events
Product: Firefox → Core
Summary: Cmd+Click on the history "back" arrow to duplicate a tab results in blank page → Docshell swapping gets message manager parent wrong
Thankfully it looks like b2g doesn't swap docshells.
(In reply to Bill McCloskey (:billm) from comment #8) > I think the main conclusion to draw from this bug is that docshell swapping > totally sucks. Is there any way we can get rid of it? Moving tabs between > windows seems like the hard case. We could at least try to not swap docShells everytime when opening tabs. I currently don't know any other way of preloading content, though. We wouldn't need that if we had two processes for UI and content, OMT animations for the tab strip, or a general-purpose content preloader, etc. We will probably not have any of those things in the next month or two. I'm all ears for any ideas wrt preloading content that don't involve docShell swapping :)
Swapping docshells shouldn't cause problems, at least not from parent point of view. Only swapping docshells between windows. But if some script relies on swapping not happen...
Component: DOM: Events → Document Navigation
I ended up needing more changes here because SetCallback was doing a few extra things. I've refactored the code to avoid that when it's not needed.
Comment on attachment 8364100 [details] [diff] [review] testcase I'd like to check this in too.
Attachment #8364100 - Flags: review?(bugs)
Comment on attachment 8368070 [details] [diff] [review] msg-mgr-fix Thanks for cleaning this up.
Attachment #8368070 - Flags: review?(bugs) → review+
Comment on attachment 8364100 [details] [diff] [review] testcase Could you add still message listeners flo1.messageManager and flo2.messageManager to make sure they get the right messages. Right now you test only window level mms.
Attachment #8364100 - Flags: review?(bugs) → review+
Comment on attachment 8368070 [details] [diff] [review] msg-mgr-fix I'd like to take this on Aurora before the merge (or on beta if we miss the date). This is a bug in an API that we're hoping a lot of add-ons will start to use. It would be great to backport this so that these add-ons don't break in unexpected ways. We also use this API in Firefox. It's possible that the bug is causing problems for us, although I don't know of any specific examples. [Approval Request Comment] Bug caused by (feature/regressing bug #): pre-FF4 User impact if declined: Some add-ons may break. Testing completed (on m-c, etc.): On m-c. Risk to taking this patch (and alternatives if risky): Low; patch is simple. String or IDL/UUID changes made by this patch: None
Attachment #8368070 - Flags: approval-mozilla-aurora?
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment on attachment 8368070 [details] [diff] [review] msg-mgr-fix Go ahead for aurora!
Attachment #8368070 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Doesn't apply cleanly to Aurora. Please provide a branch-specific patch.
Thanks Ryan. This applies cleanly to aurora and compiles locally for me.
Flags: needinfo?(wmccloskey) → needinfo?(ryanvm)
You need to log in before you can comment on or make changes to this bug.