Closed Bug 962767 Opened 6 years ago Closed 6 years ago

Docshell swapping gets message manager parent wrong

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox28 --- fixed
firefox29 --- fixed

People

(Reporter: ttaubert, Assigned: billm)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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.
With bug 961861 backed out I can't reproduce it anymore.
Blocks: 961861
No longer blocks: 942374
Attached patch testcaseSplinter Review
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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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.
Attached patch frame-loader-parent (obsolete) — Splinter Review
I think this does what you want.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8364162 - Flags: review?(bugs)
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...
Attachment #8364162 - Flags: review?(bugs) → review+
Component: DOM: Events → Document Navigation
Attached patch msg-mgr-fixSplinter Review
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.
Attachment #8364162 - Attachment is obsolete: true
Attachment #8368070 - Flags: review?(bugs)
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?
https://hg.mozilla.org/mozilla-central/rev/f55a6150f6ee
https://hg.mozilla.org/mozilla-central/rev/df918fe43131
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
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.
Flags: needinfo?(wmccloskey)
Attached patch aurora patchSplinter Review
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.