962767 - Docshell swapping gets message manager parent wrong

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Maybe also bug 961861?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

I see the same error when opening and closing some blank tabs using Cmd+T/W.

Comment 3

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

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 4

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Now even comment #0 isn't happening anymore, weird. I didn't even close the browser in the meantime.

Comment 6

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

OK, I can get the exception by opening and closing tabs a lot.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

With bug 961861 backed out I can't reproduce it anymore.

Comment 8

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

Attachment: testcase

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.

Comment 9

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

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.

Comment 10

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

er, wait... /me reads some code :)

Comment 11

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

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.

Comment 12

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

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.

Comment 13

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

Attachment: frame-loader-parent

I think this does what you want.

Comment 14

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

Thankfully it looks like b2g doesn't swap docshells.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(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 :)

Comment 16

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

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...

Comment 17

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

Attachment: msg-mgr-fix

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 18

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

Comment on attachment 8364100 [details] [diff] [review]
testcase

I'd like to check this in too.

Comment 19

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

Comment on attachment 8368070 [details] [diff] [review]
msg-mgr-fix

Thanks for cleaning this up.

Comment 20

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

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.

Comment 21

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f55a6150f6ee
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/df918fe43131

Comment 22

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

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f55a6150f6ee
https://hg.mozilla.org/mozilla-central/rev/df918fe43131

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8368070 [details] [diff] [review]
msg-mgr-fix

Go ahead for aurora!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply cleanly to Aurora. Please provide a branch-specific patch.

Comment 26

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

Attachment: aurora patch

Thanks Ryan. This applies cleanly to aurora and compiles locally for me.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d5f71dffacf