Build ID: 2000080108 M18 Win98 Steps to Reproduce: (1) Visit a website. (2) Switch themes. (3) Press reload to reload the website. Result: The website doesn't reload; the reload button does nothing. You can still reload by pressing enter in the URL bar again, and the reload button starts functioning once you navigate elsewhere. Split off from bug 45663, I didn't know if you wanted to take this one also dan.

Yeah. Theme switching is very invasive right now. Many things are tossed and replaced by shiny new empty things. One of those is docshell history. Another is, for instance, anything and everything you may have typed into a composer window. This wants to be better.

submitting for nsbeta3

Handling theme switches properly is the responsiblity of the individual component owners. Style sheet changes are a similar scenario. ->radha for triage.

heh! So, one can break an existing behavior while implementing a new feature and don't bother to fix the breakage! Worst, don't even notify the respective people of the breakage coming or seek input in fixing it? Now, looking thro' the comments in the parent bug, 45663, the following suggestion from rpotts is what that needs to happen... The *real* problem is that when the skin is changed, the BrowserInstance is not notified... If it were, it could re-cache its pointer to the docshell, hook up session history again, and cause the Content area to be reloaded... Looking at the hack in nsBrowserInstance::GetContentDocShell() , it looks like part of what is said above happens. But I don't think any kind of *real* notification is done to BrowserInstance when the content docshell changes. A real notification is needed so that browser can decide if it has to set the handle to session History again in the docshell.

This is not unique to skin switching. Frames are recreated when style sheets are dynamically added or removed from a document as well. This is all skin switching is doing, adding and removing style sheets (something you can also do through the CSS DOM).

Hey redha, why had you removed some people from CC?

Resetting the component to Embedding-DocShell. Either the DocShell needs to notify nsIBrowserInstance when it's being subjected to whatever abuse it's receiving, or, nsPresShell.cpp shouldn't subject it to that abuse. I'll let the owner of that component decide which it is.

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

Please see bug 47930 regarding a session history ramification to this bug.

That should of course be bug 47490, only the last few comments are relevant.

skin triage team (hyatt, hangas, ben, johng, tpringle): this is essential for skin switching, so nsbeta3+

updating summary to include another result of not preserving SH when theme switching (from bug 47490)

Adding 'crash' keyword to all open crashers. Please correct any mistakes. Thanks!

spam: adding self and pmac.

Adding topcrash keyword. I believe, based on user comments, that this crash accounts for the talkback crashes in: nsDocShell::AddRef nsWebShell::AddRef PR_AtomicIncrement _PR_MD_ATOMIC_INCREMENT

If dbaron's comment is correct, I see an error with that type of stack trace under Purify involving a chain of free memory reads and writes during exit of viewer. I'll try and see if I can get something similar out of the browser.

I have a fix that almost works. None of the history buttons menus crash. But the Go menu/Back/Forward menus may *sometimes* give you wrong entries. This is because, after switching themes, the docshell handle that BrowserInstance holds on to, mContentAreaDocShellWeak is updated *only* when nsBrowserInstance::GetContentAreaDocShell() is called. THis function gets called for Back/forward/Reload buttons, loading url from the urlbar or selecting from a bookmark. So all these buttons behave ight. But the back/forward/go menu items don't call this function. So, they may show wrong menu items depending on when you invoke them. I think the right way to fix this is, whenever themes are switched, nsBrowserInstance should get notified some how and in that notification, we should update the handle to docshell/uriloader/window etc... and then all will be well. I will post some patches soon that will demonstrate what I have described.

I assume that when back/forward/reload are converted from nsIBrowserInstance to nsIWebNavigation, this would "break" them too, so yes, that really needs to be fixed. I'm not sure about your suggested fix though, since ultimately the SessionHistory fetched from the browser's docshell should be up to date after skin switching (dynamically adding/removing stylesheets), which is where things go wrong currently.

Adam, has any work been done on this?

This has been fixed. No more crashes. There could be a mis-behavior with go/back/forward menus (just the menus, not the button) after switching themes. I'm working on it.

I don't see any crashes either. You do lose the current page when you switch themes, but history and reload function correctly.

I am removing the nsbeta3+ for reevaluation; now that the crashes are gone, I don't think this is a beta3 stopper anymore. Radha: is history still lost, and is the reload button still not working properly?

blake, it's just like radha and adam said. With the 2000091312 builds after switching themes the current page seems to disappear but CAN be restored by clicking reload. Session History is also unaffected by switching themes. So, if there is another bug for losing the current page this bug can probably go to fixed/WFM.

...I don't see where Radha or Adam mentioning the loss of history or the reload button problem. They just said it doesn't crash anymore. Also, Radha mentioned some possible strange behavior with the menus, but I guess that's not within the scope of this bug. Yes, there's a bug on the webpage disappearing -- bug 44437. Marking this fixed.

VERIFIED Fixed with 2000091408 builds