Closed Bug 330089 Opened 18 years ago Closed 18 years ago

[FIX]crash when retrieving contentFrame.contentDocument

Categories

(Core :: DOM: Core & HTML, defect, P1)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: eyalroz1, Assigned: bzbarsky)

References

(Depends on 1 open bug,
URL
)

Details

(Keywords: crash, helpwanted)

Attachments

(3 files, 1 obsolete file)

Stack showing part of the problem
28.10 KB, text/plain
Details
Testcase to not break (shouldn't assert or lead to similar crashes)
166 bytes, text/html
Details
Proposed patch
6.31 KB, patch
Biesinger
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review 
In the last couple of nightlies, in:

http://www.mozdev.org/source/browse/~checkout~/bidiui/source/tbird/chrome/content/bidimailpack/bidimailpack-composer.js?rev=1.137&content-type=text/plain

from the code of BiDi Mail UI has been triggering a crash; we have determined that the crash occurs when retrieving contentFrame.contentDocument, on the line

        direction =
          document.defaultView
                  .getComputedStyle(document.getElementById("content-frame")
                  .contentDocument.body, "").getPropertyValue("direction");

in the setCasterPair function of the directionSwitchController.
Bug is first triggered with the nightly from 2006-03-07.
To reproduce:

1. Install the extension

http://downloads.mozdev.org/bidiui/latest/bidimailui_suite.xpi

2. close sm
3. start sm mail & news
4. press 'compose'
5. crash and burn!
Talkback:

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB16204763K

Stack:

0x00000000
nsIFrame::SetStyleContext  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/../generic\nsIFrame.h, line 558]
nsFrameManager::ReResolveStyleContext  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsFrameManager.cpp, line 1312]
nsFrameManager::ComputeStyleChangeFor  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsFrameManager.cpp, line 1351]
nsCSSFrameConstructor::RestyleElement  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 10610]
nsCSSFrameConstructor::ProcessOneRestyle  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13435]
nsCSSFrameConstructor::ProcessPendingRestyles  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 13487]
PresShell::FlushPendingNotifications  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp, line 5060]
nsBoxObject::GetFrame  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxObject.cpp, line 163]
nsEditorBoxObject::GetDocShell  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsEditorBoxObject.cpp, line 73]
XPCWrappedNative::CallMethod  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2152]
XPC_WN_GetterSetter  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1476]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1246]
js_InternalInvoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1347]
js_InternalGetOrSet  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1406]
js_GetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c, line 3022]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 5614]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1270]
js_InternalInvoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1347]
js_InternalGetOrSet  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1406]
js_GetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c, line 3022]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 5614]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1270]
js_InternalInvoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1347]
js_InternalGetOrSet  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1406]
js_GetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c, line 3022]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 5614]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1270]
js_InternalInvoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1347]
js_InternalGetOrSet  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1406]
js_GetProperty  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsobj.c, line 3022]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 5614]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1270]
nsXPCWrappedJSClass::CallMethod  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp, line 1379]
nsXPCWrappedJS::CallMethod  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp, line 466]
SharedStub  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp, line 147]
XPTC_InvokeByIndex  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2152]
XPC_WN_CallMethod  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1444]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1246]
js_Interpret  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 3886]
js_Invoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1270]
js_InternalInvoke  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsinterp.c, line 1347]
JS_CallFunctionValue  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/js/src/jsapi.c, line 4189]
nsJSContext::CallEventHandler  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1426]
nsJSEventListener::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/events/nsJSEventListener.cpp, line 195]
nsEventListenerManager::HandleEventSubType  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp, line 1653]
nsEventListenerManager::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventListenerManager.cpp, line 1760]
nsEventTargetChainItem::HandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 335]
nsEventTargetChainItem::HandleEventTargetChain  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 478]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 405]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventTargetChainItem::CreateChainAndHandleEvent  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 392]
nsEventDispatcher::Dispatch  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/events/src/nsEventDispatcher.cpp, line 575]
nsXULCommandDispatcher::UpdateCommands  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/content/xul/document/src/nsXULCommandDispatcher.cpp, line 415]
nsGlobalWindow::UpdateCommands  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsGlobalWindow.cpp, line 4499]
nsFocusController::UpdateCommands  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsFocusController.cpp, line 217]
nsFocusController::SetSuppressFocus  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/dom/src/base/nsFocusController.cpp, line 513]
PresShell::UnsuppressPainting  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/base/nsPresShell.cpp, line 4796]
nsDocShell::EndPageLoad  [c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/docshell/base/nsDocShell.cpp, line 4743]
OK, I followed the steps in comment 4 with a current trunk Linux debug build.  I saw no changes to the UI (should I have?) and didn't get a crash.

That said, the stack is kinda useful.  Makes me suspect that bug 329181 is to blame.  Hard to tell how, though, without having a way to reproduce the crash.  :(  Any ideas on how I can do that?  How do I tell whether the extension is correctly installed, for one thing?
Uh, note I said platform = windows XP. Dunno about Linux.
Well, I can't test on Windows, unfortunately....  Does that extension not work on other OSes?  Again, how do I tell whether the extension is properly installed?
Oh yes, I forgot to mention you must be composing a text message, not an HTML message. Screenshot of the composition window with BiDi Mail UI operational:

http://bidiui.mozdev.org/images/new-message-ltr.png

And the extension does work on both Linux and Mac.
Flags: blocking1.9a1?
QA Contact: ian → bugs.mano
Aha!  There we go.  With text composition I do see the crash.  I was using a default profile for testing, as usual.  ;)

Well, the good news (for me) is that this is not a regression from bug 329181 (tested by local backout).

The bad news is that I'm not sure what it's a regression from.  ;)
OK, testing by local backout indicates the problem is bug 234455.

Why, I'm not sure.  But the immediate cause of the crash is that we end up with a dead frame in ReResolveStyleContext somehow, which of course crashes....  :(
Blocks: 234455
OS: Windows XP → All
Hardware: PC → All
I added an assert that fires if nsFrame::Destroy is called while we're in the middle of style reresolution.  That happens in this case.  Basically, a child docloader fires its onload event (frame 149).  This triggers some script, which tries to get at docshell of an editor.  That triggers a style reresolve on the parent document, which causes a background image load to start and synchronously(!) stop.  The stop causes the parent docloader to fire its own onload in frame 60 (since the child is no longer busy by this point), which runs more script and triggers style changes.  Then it _again_ tries to get at the docshell of our editor, which again flushes style reresolves, which destroys some frames.  That's where we assert.

If we now continue, we unwind to the outer style reresolve, but that's now working with deleted frames and we crash.

I think I can fix docloader to prevent this by preventing onload firing on the outer docloader while the inner one is in the middle of its own onload handler, but what in the event patch causes a change in behavior here?
The handling of NS_PAGE_LOAD seems to have changed.
Earlier it didn't go through even the capture phase.
There was this in the nsGenericElement (for capture phase)
aEvent->message != NS_PAGE_LOAD &&
aEvent->message != NS_SCRIPT_LOAD &&
aEvent->message != NS_IMAGE_LOAD &&
aEvent->message != NS_IMAGE_ERROR &&
aEvent->message != NS_SCROLL_EVENT
I think I need to add those back (at least temporarily), although that is wrong :(

Now I was going debug this, but I can't get this crashing.
Attached patch Prevent capture of some events (obsolete) — — Splinter Review 
So, I can't reproduce the bug, but I think we should anyway put back 
this.
See the old version
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsGenericElement.cpp&rev=3.448#1965
Attachment #214910 - Flags: superreview?(bzbarsky)
Attachment #214910 - Flags: review?(bzbarsky)
(In reply to comment #17) 
> So, I can't reproduce the bug, but I think we should anyway put back 
> this.

And if somehow who can reproduce the bug could test whether the patch helps, that'd be great.
Comment on attachment 214910 [details] [diff] [review]
Prevent capture of some events

This didn't help, and bz should have a real fix. (something not related to changes in 234455)
Attachment #214910 - Flags: superreview?(bzbarsky)
Attachment #214910 - Flags: review?(bzbarsky)
Comment on attachment 214910 [details] [diff] [review]
Prevent capture of some events

This doesn't fix the crash for me, and I don't think we want this in general...
Attachment #214910 - Attachment is obsolete: true
Depends on: 261091
Attached patch Proposed patch — — Splinter Review 
I tried to reproduce this on the 1.8 branch without XUL, but the cache validation stuff imgLoader is just too complicated for me.  In any case, any time an image is in a loadgroup and doesn't need validation and gets used for a background image we can hit this bug.

Which means that I suspect we need this on the 1.8 branches, assuming this is safe and doesn't cause regressions.  This code is not exactly happy that way.  :(
Assignee: general → bzbarsky
Status: NEW → ASSIGNED
Attachment #214942 - Flags: superreview?(darin)
Attachment #214942 - Flags: review?(cbiesinger)
Priority: -- → P1
Summary: crash when retrieving contentFrame.contentDocument → [FIX]crash when retrieving contentFrame.contentDocument
Whiteboard: helpwanted
Target Milestone: --- → mozilla1.9alpha
I will likely not get to this before monday next week (mar 20)
Wouldn't this be better solved by making LoadImage not call RemoveRequest synchronously?  I'd be happy to take on that task, especially if we are only talking about a fix for Mozilla 1.9.
> Wouldn't this be better solved by making LoadImage not call RemoveRequest
> synchronously?

Probably yes.  ;)

If you're willing to do that, though, that's great, and I agree that we should do it anyway.  I do think we might need _some_ fix for this on 1.8.1, if not 1.8.0.x, though.  :(
Or at least, I'll need some sort of argument about when LoadImage can and cannot make a sync call to RemoveRequest to convince me that we don't need this on the branches.  As things stand, we're crashing because we touch deleted objects, and that's usually a bad sign.
Comment on attachment 214942 [details] [diff] [review]
Proposed patch

+        // It's ok if we're already in the list -- we'll just be in there twice
+        // and then the RemoveObject calls from ChildDoneWithOnload will remove
+        // us.

Maybe this comment should make it clearer that each RemoveObject will only remove one element, so this is expecting a balanced number of EnteringOnload and DoneWithOnload calls.

seems ok otherwise.
Attachment #214942 - Flags: review?(cbiesinger) → review+
The behavior of the text message composition window keeps getting weirder. The direction gets switched back and forth, the view/s are not available for JS access, problems with cursor movement, etc. etc.

Something is rotten in the kingdom of composermark.
Blocks: 311963
Oh, I should mention this:

https://bugzilla.mozilla.org/show_bug.cgi?id=330958

is probably a related issue, since that's what happens when I do

var contentDocument = document.getElementById("content-frame").contentDocument;
direction = contentDocument.defaultView.getComputedStyle(contentDocument.body, "").getPropertyValue("direction");

instead of the code I posted in comment 0.
Blocks: 330958
Comment on attachment 214942 [details] [diff] [review]
Proposed patch

>Index: uriloader/base/nsDocLoader.h

>+    // Inform a parent docloader that aChild is about to call its onload
>+    // handler.
>+    PRBool ChildEnteringOnload(nsIDocumentLoader* aChild) {
>+        // It's ok if we're already in the list -- we'll just be in there twice
>+        // and then the RemoveObject calls from ChildDoneWithOnload will remove
>+        // us.
>+        return mChildrenInOnload.AppendObject(aChild);
>+    }

Should we be concerned about array growth?  Under what conditions
would this method be called more than once for a particular child?


sr=darin (sorry for not reviewing this sooner... i think it's not 
worth holding this patch for an improved imglib)
Attachment #214942 - Flags: superreview?(darin) → superreview+
Any time an onload handler causes recursion into itself, basically...  I'm not sure that's possible, but I'm not sure it's not either, so I figured I'd make the code just deal with it (and we'd crash when we overflow the stack if we really recurse infinitely).
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
No longer blocks: 330958
Flags: blocking1.9a1?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: