45663 - crash reloading page after switching to classic theme [@ DocumentViewerImpl::MakeWindow]

Status: VERIFIED FIXED

(SeaMonkey :: Themes, defect, P3)

Description

[Jay Patel [:jay]]

i'm not sure if this occurs on other platforms, but it crashes on my NT and 
Win98 machines.

steps:
1. switch theme from modern to classic
2. after the chrome is changed, the page content is blank
3. click reload
4. browser crashes

this has made it to the talkback topcrash list, so adding topcrash keyword.  
here is some data from talkback:

DocumentViewerImpl::MakeWindow da254854        
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/src/nsDocumentVi
ewer.cpp line 1054
        Build: 2000070821 CrashDate: 2000-07-09 UptimeMinutes: 20  Total: 20 
        OS: Windows 95  4.0 build 67306684
        URL: All I did was try to reload a page (netscape webmail) after  a 
theme change.
        Comment: 
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=13865974

DocumentViewerImpl::MakeWindow fc32314d
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/src/nsDocumentVi
ewer.cpp line 1054
        Build: 2000071113 CrashDate: 2000-07-11 UptimeMinutes: 5  Total: 5 
        OS: Windows 98  4.10 build 67766446
        URL: home.netscape.com
        Comment: i changed my skin from modern to classic and after doing so
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=13998994

DocumentViewerImpl::MakeWindow 9a333e58
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/src/nsDocumentVi
ewer.cpp line 1054
        Build: 2000071321 CrashDate: 2000-07-14 UptimeMinutes: 16  Total: 16 
        OS: Windows NT  4.0 build 1381
        URL: 
        Comment: switched to classic skin.  couldn't type in URL after I did 
that.  Then opened File->open web location.  Opened www.yahoo.com and never 
looked back.  CRASH
        Stacktrace: 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=14172251

Here's the stack trace:

Incident ID 13865974 
DocumentViewerImpl::MakeWindow 
[d:\builds\seamonkey\mozilla\layout\base\src\nsDocumentViewer.cpp, line 1054] 
DocumentViewerImpl::Init 
[d:\builds\seamonkey\mozilla\layout\base\src\nsDocumentViewer.cpp, line 535] 
nsDocShell::SetupNewViewer 
[d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, line 2553] 
nsWebShell::SetupNewViewer 
[d:\builds\seamonkey\mozilla\webshell\src\nsWebShell.cpp, line 564] 
nsDocShell::Embed [d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, 
line 2222] 
nsWebShell::Embed [d:\builds\seamonkey\mozilla\webshell\src\nsWebShell.cpp, line 
588] 
nsDocShell::CreateContentViewer 
[d:\builds\seamonkey\mozilla\docshell\base\nsDocShell.cpp, line 2380] 
nsDSURIContentListener::DoContent 
[d:\builds\seamonkey\mozilla\docshell\base\nsDSURIContentListener.cpp, line 101] 
nsDocumentOpenInfo::DispatchContent 
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp, line 362] 
nsDocumentOpenInfo::OnStartRequest 
[d:\builds\seamonkey\mozilla\uriloader\base\nsURILoader.cpp, line 234] 
nsHTTPFinalListener::OnStartRequest 
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHTTPResponseListener.cp
p, line 1153] 
InterceptStreamListener::OnStartRequest 
[d:\builds\seamonkey\mozilla\netwerk\cache\mgr\nsCachedNetData.cpp, line 1140] 
nsHTTPServerListener::FinishedResponseHeaders 
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHTTPResponseListener.cp
p,
line 1091] 
nsHTTPServerListener::OnDataAvailable 
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHTTPResponseListener.cp
p, line 428] 
nsOnDataAvailableEvent::HandleEvent 
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsAsyncStreamListener.cpp, line 
407] 
nsStreamListenerEvent::HandlePLEvent 
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsAsyncStreamListener.cpp, line 
106] 
PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 588] 
PL_ProcessPendingEvents [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, 
line 547] 
_md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 
1045] 
KERNEL32.DLL + 0x35d9 (0xbff735d9) 
KERNEL32.DLL + 0x2222f (0xbff9222f) 
0x00688b48

Comment 1

[rbs]

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

Comment 2

[don]

This isn't a duplicate of bug #45633.  If anything, the situation is the other
way around.  Re-opening so I can sort all these bugs out.

Comment 3

[don]

Re-assigning to Rick Potts since he also owns bug #44437 (which is probably just
a subset of the problem in this bug).

Comment 4

[Blake Ross]

I don't quite understand the way you sorted the bugs out.  This, to me, looks 
like an exact dup of bug 45633.  You marked 45633 as worksforme because it 
required a reload before crashing, but that's exactly what the reporter there 
said in his steps to reproduce.  But it's been done, so whatever.

I don't think bug 44437 is a subset of this; that's been around for awhile, 
whereas this seems like a fairly recent regression.

Comment 5

[chris hofmann]

nsbeta2?

Comment 6

[Phil Peterson]

I've had a bunch of crashes right after switching skins (e.g. bug 44807), but I
can't discern the pattern. I bet someone with Purify needs to play with this.

Comment 7

[leger]

Putting on [nsbeta2+] radar

Comment 8

[Blake Ross]

assigning QA to me

Comment 9

[leger]

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

Comment 10

[Jerry Baker]

I just put in another Talkback report for you guys on this crash. ID 
TB14509672K. Does that make the stack trace URL 
http://cyclone/reports/stackcommentemail.cfm?dynamicBBID=14509672

This is occuring under Win2k as well.

Comment 11

[Shiva Thirumazhusai]

Since the incident will be removed from talkback database after sometime. I am 
pasting  the stack trace for the reference.

Incident ID 14509672 
nsCOMPtr_base::assign_with_AddRef 
[d:\builds\seamonkey\mozilla\xpcom\base\nsCOMPtr.cpp, line 58] 
nsSHistory::LoadEntry 
[d:\builds\seamonkey\mozilla\xpfe\components\shistory\src\nsSHistory.cpp, line 
460] 
nsSHistory::Reload 
[d:\builds\seamonkey\mozilla\xpfe\components\shistory\src\nsSHistory.cpp, line 
366] 
nsBrowserInstance::Reload 
[d:\builds\seamonkey\mozilla\xpfe\browser\src\nsBrowserInstance.cpp, line 531] 
XPTC_InvokeByIndex 
[d:\builds\seamonkey\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp, 
line 139] 
nsXPCWrappedNativeClass::CallWrappedMethod 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativeclass.cpp, 
line 913] 
WrappedNative_CallMethod 
[d:\builds\seamonkey\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp, 
line 195] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 717] 
js_Interpret [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 2518] 
js_Invoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 733] 
js_InternalInvoke [d:\builds\seamonkey\mozilla\js\src\jsinterp.c, line 806] 
JS_CallFunctionValue [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 2819] 
nsJSContext::CallEventHandler 
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 850] 
nsJSEventListener::HandleEvent 
[d:\builds\seamonkey\mozilla\dom\src\events\nsJSEventListener.cpp, line 155] 
nsEventListenerManager::HandleEventSubType 
[d:\builds\seamonkey\mozilla\layout\events\src\nsEventListenerManager.cpp, line 
773] 
nsEventListenerManager::HandleEvent 
[d:\builds\seamonkey\mozilla\layout\events\src\nsEventListenerManager.cpp, line 
1653] 
nsXULElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\rdf\content\src\nsXULElement.cpp, line 3350] 
PresShell::HandleDOMEventWithTarget 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 4041] 
nsTitledButtonFrame::MouseClicked 
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsTitledButtonFrame.cpp, line 
1279] 
nsButtonBoxFrame::HandleEvent 
[d:\builds\seamonkey\mozilla\layout\xul\base\src\nsButtonBoxFrame.cpp, line 98] 
PresShell::HandleEventInternal 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 4010] 
PresShell::HandleEventWithTarget 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 3973] 
nsEventStateManager::CheckForAndDispatchClick 
[d:\builds\seamonkey\mozilla\layout\events\src\nsEventStateManager.cpp, line 
1775] 
nsEventStateManager::PostHandleEvent 
[d:\builds\seamonkey\mozilla\layout\events\src\nsEventStateManager.cpp, line 
854] 
PresShell::HandleEventInternal 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 4014] 
PresShell::HandleEvent 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 3928] 
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 782] 
nsViewManager2::DispatchEvent 
[d:\builds\seamonkey\mozilla\view\src\nsViewManager2.cpp, line 1389] 
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 69] 
nsWindow::DispatchEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 564] 
nsWindow::DispatchWindowEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 581] 
nsWindow::DispatchMouseEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3689] 
ChildWindow::DispatchMouseEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3894] 
nsWindow::ProcessMessage 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 2809] 
nsWindow::WindowProc 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 830] 
USER32.DLL + 0x3eb0 (0x77e13eb0) 
USER32.DLL + 0x401a (0x77e1401a) 
USER32.DLL + 0x92da (0x77e192da) 
nsAppShellService::Run 
[d:\builds\seamonkey\mozilla\xpfe\appshell\src\nsAppShellService.cpp, line 387] 
main1 [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 920] 
main [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1095] 
WinMain [d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp, line 1111] 
WinMainCRTStartup() 
KERNEL32.DLL + 0x7903 (0x77e87903) 

Comment 12

[Scott MacGregor]

adding myself to the cc list. I may be able to help debug this once i bounce of
zero bugs. 

Comment 13

[Nisheeth Ranjan]

Taking...

Comment 14

[Nisheeth Ranjan]

Adding rpotts to the cc list in case he watches this bug while he's on vacation.  
:-)

Comment 15

[rpotts (gone)]

I don't think this is a SH bug, it is really a bug in the way skin switching is 
implemented...

Here's what's going on (I think)...

When the skin is changed, the content DocShell is destroyed and a new one is 
created...  Unfortunately, the nsBrowserInstance is not notified of the new 
DocShell.

If a new URL is entered via the URL Bar, then 
nsBrowserInstance::GetContentAreaDocShell() causes the new DocShell to get 
reinitialized and everything is fine (???)

However, if the reload button is pressed, the SessionHistory component is asked 
to reload the page (without calling GetContentAreaDocShell())- it still has a 
non-reference-counted pointer to the dead DocShell and it crashes...

The braindead fix is to call GetContentAreaDocShell() before making the call to 
Reload(...) but this only fixes the crash.

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

Right now, the content area is empty until you either type in a new URL or crash 
hitting the reload key (which is pretty lame)

Comment 16

[rpotts (gone)]

Bug #44437 is just another symptom of the problem...  

The reason we are crashing on reload now, is because we used to call 
GetContentAreaDocShell() before doing the reload...  Since the new SH code 
landed, we no longer do this :-(

Please fix the cause, rather than just added a bogus call to 
GetContentAreaDocShell() :-)

Comment 17

[Blake Ross]

I think we should use rpott's temporary fix to fix the crash on reload, and 
throw the data loss warning that simon mentioned, for beta2.

Comment 18

[Scott MacGregor]

This may sound stupid, but wouldn't things be a lot simpler, if when we switch
skins we only reload docshells that contain chrome? Why even mess around with
destroying the content docshell in the browser window and then reloading the url
in it? You hurt performance, and cause problems like this one where the browser
instance doesn't know the content docshell has gone away and been reloaded.

Comment 19

[Scott MacGregor]

Tracing this through a little more I'm less ignorant (I think =)). I see that
the chrome registry flushes the style sheet cache and reloads the style sheet
urls associated with the document (in this case navigator.xul). This causes a
StyleSheetAdded notification via a nsIDocumentObserver interface. the pres
shell's implementation of this call invokes ReconstructDocElementHierarchy in
response to getting notification that style sheets have been updated.

This method is the code that does the work of destroying our current docshell
hierarchy (which includes the content area docshell) and reconstructs it. That's
how changing the skin causes us to destroy and reload urls for all our content
docshells.

I wonder if there's a better method in layout we can be calling in response to
the style sheet added call. It seems like we are doing more work than we should
be by destroying and reloading the frame hiearchy. Especially since docshells in
that hierarchy don't even inherit or are effected by this style sheet change. So
it seems wasteful to tear it down and recreate it. 

Comment 20

[Nisheeth Ranjan]

Dan has agreed to look at this until I return from the XML Query meeting on 
Wednesday.  Most probably this will get fixed by then because Rick Pott's has 
identified the problem and suggested short and long term solutions.

Thanks a lot, Dan!

Comment 21

[Scott MacGregor]

I don't think Rick actually advocated the short term solution. I think he was
against it as I'm reading his comments. 

Comment 22

[Scott MacGregor]

In nsPresShell.cpp, in various methods responding to style rules being added,
there is code which says:
  // XXX For now reconstruct everything
  return ReconstructFrames();

This implies that the writer actually believes whe shouldn't be reconstructing
all the frames, only the parts that are effected by the change. Adding some
granularity so we don't try to reconstruct all the frames here in this code
would fix this problem IMHO as we could prevent ourselves from tearing down
frames that aren't effected by the new style rules caused by the theme change.

Comment 23

[Dan M]

Hmmm. There is the thing about this being an nsbeta2 emergency bug. I'm not much 
inclined to figure out how to teach the style system how to be a better citizen. 
I'm more inclined to follow the path already used to solve this same problem in 
nsBrowserInstance: a weak reference to the nsIDocShell.

Comment 24

[Blake Ross]

hyatt just checked in a fix for this

Comment 25

[Dan M]

^*@#$(*& Collision. Just lost all my changes to this bug report. Fine. Yes, 
there's a fix checked in.

Comment 26

[Sean Richardson]

Testing the steps with 2000-08-01-04-M17 on WinNT, there is no crash,
but in step 3, pressing [Reload] does nothing. This happens immediately
after switching to either the classic or modern theme, and applies to all
windows. Clicking in the URL bar and typing [ENTER] does work to redisplay 
the page. Don't know whether this should be considered a new bug, or if it
is a known side-effect of a short-term fix that will get seen to for sure.

Comment 27

[Claudius Gayle]

If "hyatt checked in a fix" then he, or someone familiar w/ the fix needs to state in this bug what the fix was.
Only then can us QA types access the validity of the fix as compared to user percievable results. Based on Sean's
comments this bug probably needs to be reopened but I for one can't really tell w/out that info.

Comment 28

[Blake Ross]

Well, it's really a separate issue.  This bug was to track the crash, and was 
marked nsbeta2+ to remove the crash.  I've split off the 'reload doesn't work' 
problem as bug 47345.

Marking VERIFIED FIXED:
- win2k 2000080205 M17
- win98 2000080108 M18
- RH6.2 2000080204 M17
- RH6.2 2000080208 M18
- MacOS9 2000080204 M17
- MacOS9 2000080210 M18

Comment 29

[Jerry Baker]

Mass removing self from CC list.

Comment 30

[Jerry Baker]

Now I feel sumb because I have to add back. Sorry for the spam.