47490 - Crash when opening Go menu after switching themes

Status: VERIFIED DUPLICATE of bug 47345

(Core Graveyard :: Skinability, defect, P3)

Description

[Blake Ross]

Build ID: 2000080308 M18 win98

If you switch themes and then open the Go menu, you crash.  This is caused by 
the checkin late last night to turn on the checkmarks for the go menu, and to 
handle the back/forward/go menu creation in JS.

I think the reason for the crash is that we get the docshell on startup (via 
getContentAreaDocshell() ) and continuously use that when opening the Go menu, 
but then the docshell changes once you switch skins, and the Go menu still has 
a handle to the old docshell.

Don't know if this is my fault or not (if it is, reassign back to me).  Should 
the docshell be updated when switching skins?  I don't know where else to get a 
handle to the docshell such that it's the correct one each time.  If you get it 
every time you open the Go menu (which is a lot of overhead already), you get a 
crash the fifth or sixth time opening the menu.

Comment 1

[Blake Ross]

So, options are to get the docshell each time the Go menu is opened, or get a 
handle to the new docshell after theme switching.   I would much prefer the 
latter, since it involves getting the docshell only when we need one.  However, 
as I noted, that crashes on the fifth or sixth time, which might as well be a 
bug in itself (why should it crash just getting a reference to a docshell?)

Comment 2

[Radha on family leave (not reading bugmail)]

Blake you may want to disable this feature until you figure the crash or find a 
way to get current docshell when themes switch. 

Comment 3

[Peter "jag" Annema]

Unfortunately, there's another problem here.

I changed the code back to getting the docshell each time in 
sessionHistoryUI.js, FillBrowserMenu, but now Mozilla crashes when you open the 
Go menu (i.e. get a docshell reference), then switch themes. This doesn't happen 
with a 07-29-08 build, so I suspect it's because these references aren't cleanly 
dealt with (when gotten through JS?).

The best current work-around seems to be switching back to the 
appCore.updateGoMenu and appCore.backButtonPopup & appCore.forwardButtonPopup. 
I'll attach patches for that.

Comment 4

[Peter "jag" Annema]

"This doesn't happen with a 07-29-08 build, so I suspect it's because these 
references aren't cleanly dealt with (when gotten through JS?)."

Forgot to mention that I suspect that because between then and the 08-03-08 
build changes were made to nsBrowserInstance::GetContentAreaDocShell() 
(also see bug 46556) which I think affect this case or perhaps trigger a bug 
which was masked before.

Comment 5

[Peter "jag" Annema]

Attachment: [patch] navigator.js: use appCore again instead of FillHistoryMenu :-(

Comment 6

[Peter "jag" Annema]

Attachment: [patch] sessionHistoryUI.js: use appCore again instead of FillHistoryMenu :-(

Comment 7

[Radha on family leave (not reading bugmail)]

Blake, see if something like this will work well before you give up. 
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/viewsourc
e.js#29

Otherwise give the bug back to me and I'll dig more. I do think that this should 
go in for beta3.

Comment 8

[Blake Ross]

*** Bug 47798 has been marked as a duplicate of this bug. ***

Comment 9

[Peter "jag" Annema]

radha, I've got a fix in hand to get the checkmarks working, but it depends on 
some other code that needs fixing. Hopefully some time before Wednesday, and 
this time without the "crash on window close" & "crash on/after switch theme" 
;-)

Comment 10

[Radha on family leave (not reading bugmail)]

peter, do you want me to try your patch? What bug does you patch depends on?

Comment 11

[Peter "jag" Annema]

I think this bug can be considered fixed.
The problem was caused by some missing refcounting in
nsBrowserInstance::GetContentDocShell which was exposed by JS code in
sessionHistoryUI.js. The same thing caused bug 47491, btw.

Anyway, GetContentDocShell has been fixed, and I've attached a patch to bug
47856 which doesn't use that code and doesn't cause crashes for me upon
switching. Making this bug depend on that one 'coz we'll need the Go menu
history working again before we can test that this one is fixed too :-)

Comment 12

[David Hyatt]

So fixed? Ok.

Comment 13

[Blake Ross]

not yet. we're investigating.

Comment 14

[Peter "jag" Annema]

*sigh*
I'll have to find out what changed recently, we're crashing again, this time on
a different point though.
Start mozilla, switch skins, open go menu, crash. It doesn't crash if you just
start mozilla and open the go menu, so skin switching has something to do with
it. Back/forward dropdown history uses the same code and crashes to.

We segfault in line 43 in sessionHistoryUI.js:
var webNavigation = docShell.QueryInterface(Components.interfaces.nsIWebNavigation);

Comment 15

[Peter "jag" Annema]

Mental note: first coffee, then bugzilla.

Line 43:
var shistory = webNavigation.sessionHistory;

Is where it segfaults. I'm pretty sure it didn't crash here when I created and
tested my patch, but I'm slowly starting to suspect my memory is lying to me.

Anyway, as far as I can tell this is all legit javascript code, so any idea
what's causing the segfault, and why only after skin switching?

Comment 16

[Peter "jag" Annema]

Hmmm, so what seems to be happening is an infinite loop in
nsDocShell::GetSessionHistory:

the mSessionHistory should be set (I think), but it isn't, so it asks a root of
the same type as itself for its SessionHistory, only that root turns out to be
itself (I think), so it's asking itself ad infinitum (strategically placed
NS_WARNINGs confirm this part at least) which turns out to be till it segfaults.

Comment 17

[Peter "jag" Annema]

So from what I gather, during skin switching docshells are being destroyed and
recreated, including the <xul:browser> one (why?). Before it's destroyed,
someone should get the sessionHistory and probably some other stuff, destroy,
create, and put sessionHistory and that other stuff back in.

Comment 18

[Radha on family leave (not reading bugmail)]

In any case I don't think nsDocShell::GetSessionHistory should be going in a 
infinite loop until it segfaults when it does not find a session History. I will 
look in to it.  But to fix this problem, like peter has explained above, someone 
needs to set the session history back. in the new docshell.

Comment 19

[Stephen Walker]

*** Bug 49086 has been marked as a duplicate of this bug. ***

Comment 20

[Peter Trudelle]

reassigning to radha per hyatt.

Comment 21

[Bill Law]

Resolving as a dup of 47345.  The fix for that bug must preserve the session 
history (or so it seems to me).  I'll mention that there.

*** This bug has been marked as a duplicate of 47345 ***

Comment 22

[Blake Ross]

vrfy dup, will recheck this once that gets fixed.

Comment 23

[Blake Ross]

*** Bug 49255 has been marked as a duplicate of this bug. ***