User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0 Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0 Description: The attached testcase crashes Firefox 4.0 on Linux and Windows. Marked as potential security issue as FF is accessing memory in unmapped regions. Affected Versions: Firefox 4.0 (Windows and Linux) Testcase: The testcase is attached as an HTML file. It will crash the browser on opening. Assertions: Following assertions are triggered before the process crashes on Linux: ###!!! ASSERTION: Dying in the middle of our own update?: 'mUpdateCount == 0', file ../../../layout/base/nsCSSFrameConstructor.h, line 89 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 7365 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: too many EndUpdate's: '0 != mUpdateCount', file ../../../layout/base/nsPresShell.cpp, line 3401 ###!!! ASSERTION: Should be in an update while creating frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 6488 ###!!! ASSERTION: too many EndUpdate's: '0 != mUpdateCount', file ../../../layout/base/nsPresShell.cpp, line 3401 ###!!! ASSERTION: Should be in an update while creating frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 6488 ###!!! ASSERTION: too many EndUpdate's: '0 != mUpdateCount', file ../../../layout/base/nsPresShell.cpp, line 3401 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 7365 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Should be in an update while destroying frames: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1442 ###!!! ASSERTION: Instant quote updates are bad news: 'mUpdateCount != 0', file ../../../layout/base/nsCSSFrameConstructor.h, line 1788 ###!!! ASSERTION: recurring into frame construction: 'mPresContext->mLayoutPhaseCount[eLayoutPhase_FrameC] == 0', file ../../../layout/base/nsPresContext.h, line 1375 Program received signal SIGSEGV, Segmentation fault. 0xf597c5c6 in ?? () from ./libxul.so Stack Backtrace: Following crashes have been observed: Windows / Firefox 4.0: (127c.838): Access violation - code c0000005 (first chance) First chance exceptions are reported before any exception handling. This exception may be expected and handled. xul!nsBlockInFlowLineIterator::nsBlockInFlowLineIterator+0x8c: 6c9c7f7c 8b4020 mov eax,dword ptr [eax+20h] ds:002b:f0de801f=???????? xul!nsBlockInFlowLineIterator::nsBlockInFlowLineIterator(class nsBlockFrame * aFrame = 0x09819df8, class nsIFrame * aFindFrame = 0x00000001, int * aFoundValidLine = 0x00000000)+0x8c xul!nsBlockFrame::ChildIsDirty(class nsIFrame * aChild = 0x00000000)+0x2e xul!PresShell::FrameNeedsReflow(class nsIFrame * aFrame = 0x09819b00, nsIPresShell::IntrinsicDirty aIntrinsicDirty = eStyleChange (2), unsigned int64 aBitToAdd = 0x400)+0x6c4 xul!nsTextFrame::CharacterDataChanged(struct CharacterDataChangeInfo * aInfo = 0x00000000)+0x7c xul!nsCSSFrameConstructor::CharacterDataChanged(class nsIContent * aContent = 0x001fbec8, struct CharacterDataChangeInfo * aInfo = 0x001fc090)+0x65 xul!PresShell::CharacterDataChanged(class nsIDocument * aDocument = 0x6c969f6e, class nsIContent * aContent = 0x001fc090, struct CharacterDataChangeInfo * aInfo = 0x6d3fde1c)+0x63 xul!nsNodeUtils::CharacterDataChanged(class nsIContent * aContent = 0x09819df8, struct CharacterDataChangeInfo * aInfo = 0x001fc090)+0x85 xul!nsGenericDOMDataNode::SetTextInternal(unsigned int aOffset = 0, unsigned int aCount = 1, wchar_t * aBuffer = 0x0c35c6e8 "“", unsigned int aLength = 1, int aNotify = 1)+0x20e xul!nsGenericDOMDataNode::SetData(class nsAString_internal * aData = 0x6d4fe160)+0x20 xul!nsCommentNode::SetData(class nsAString_internal * aData = 0x6d4fe160)+0x11 xul!nsQuoteList::RecalcAll(void)+0x48 xul!PresShell::EndUpdate+0x2e2809 xul!nsTextEditorState::SetValue(class nsAString_internal * aValue = 0x001fc398, int aUserInput = 0)+0x4e0 xul!nsTextEditorState::UnbindFromFrame(class nsTextControlFrame * aFrame = 0x098191d0)+0x408 xul!nsHTMLInputElement::UnbindFromFrame(class nsTextControlFrame * aFrame = 0x0e1e7168)+0x2e xul!nsTextControlFrame::DestroyFrom(class nsIFrame * aDestructRoot = 0x0e1e7168)+0x31 xul!nsBlockFrame::DestroyFrom(class nsIFrame * aDestructRoot = 0x0e1e7168)+0x1bd xul!nsFrameList::DestroyFramesFrom(class nsIFrame * aDestructRoot = 0x6c981d66)+0x1c xul!nsCanvasFrame::DestroyFrom(class nsIFrame * aDestructRoot = 0x0e1e7168)+0x12 xul!nsContainerFrame::DestroyFrom(class nsIFrame * aDestructRoot = 0x00000000)+0x46 xul!nsHTMLScrollFrame::DestroyFrom(class nsIFrame * aDestructRoot = 0x6d01110c)+0x17 xul!nsFrameList::DestroyFrameIfPresent(class nsIFrame * aFrame = 0x001fbec8)+0x13 xul!nsContainerFrame::RemoveFrame(class nsIAtom * aListName = 0x00000001, class nsIFrame * aOldFrame = 0x00000000)+0x4f xul!ViewportFrame::RemoveFrame(class nsIAtom * aListName = 0x00000000, class nsIFrame * aOldFrame = 0x0e1e7168)+0x2a xul!nsFrameManager::RemoveFrame(class nsIAtom * aListName = 0x6c9ab681, class nsIFrame * aOldFrame = 0x00000000)+0x27 xul!nsCSSFrameConstructor::ContentRemoved(class nsIContent * aContainer = 0x00000000, class nsIContent * aChild = 0x0c3e9f10, class nsIContent * aOldNextSibling = 0x00000000, nsCSSFrameConstructor::RemoveFlags aFlags = REMOVE_FOR_RECONSTRUCTION (1), int * aDidReconstruct = 0x001fc5a0)+0x230 xul!nsCSSFrameConstructor::RecreateFramesForContent(class nsIContent * aContent = 0x0c3e9f10, int aAsyncInsert = 1)+0x12a xul!nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval+0x29e526 xul!nsCSSFrameConstructor::RecreateFramesForContent(class nsIContent * aContent = 0x0e0ce250, int aAsyncInsert = 205430547)+0xcb xul!nsCSSFrameConstructor::ReframeContainingBlock(class nsIFrame * aFrame = 0x0cf4d440)+0x3e xul!nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval+0x29e3bb xul!nsCSSFrameConstructor::ProcessRestyledFrames+0x3af7f4 xul!mozilla::css::RestyleTracker::ProcessRestyles(void)+0x30b xul!nsCSSFrameConstructor::ProcessPendingRestyles(void)+0x24 xul!PresShell::FlushPendingNotifications(mozFlushType aType = Flush_InterruptibleLayout (4))+0x1f7 xul!PresShell::WillPaint(int aWillSendDidPaint = 145104816)+0x3d xul!nsViewManager::DispatchEvent(class nsGUIEvent * aEvent = 0x001fd190, class nsIView * aView = 0x08aab1c0, nsEventStatus * aStatus = 0x001fd07c)+0x26d xul!AttachedHandleEvent(class nsGUIEvent * aEvent = 0x6c8e81a1)+0x76 xul!nsWindow::DispatchEvent(class nsGUIEvent * event = 0x00000000, nsEventStatus * aStatus = 0x00000000)+0x36 xul!nsWindow::DispatchWindowEvent(class nsGUIEvent * event = 0x00000000)+0x13 xul!nsWindow::OnPaint(struct HDC__ * aDC = 0x00000000, unsigned int aNestingLevel = 0)+0xf1 xul!nsWindow::ProcessMessage(unsigned int msg = 0xf, unsigned int * wParam = 0x001fd458, long * lParam = 0x001fd45c, long * aRetValue = 0x001fd444)+0x9c xul!nsWindow::WindowProcInternal(struct HWND__ * hWnd = 0x00000000, unsigned int msg = 0, unsigned int wParam = 0, long lParam = 0)+0xde xul!CallWindowProcCrashProtected(<function> * wndProc = <Memory access error>, struct HWND__ * hWnd = <Memory access error>, unsigned int msg = <Memory access error>, unsigned int wParam = <Memory access error>, long lParam = <Memory access error>)+0x2e Linux / Firefox 4.0: Program received signal SIGSEGV, Segmentation fault. 0xf6e976e0 in ?? () from ./libxul.so (gdb) info reg eax 0x1 1 ecx 0x2 2 edx 0xf370c7ff -210712577 ebx 0xf7f34ebc -135049540 esp 0xffffb55c 0xffffb55c ebp 0xe3fbe6c0 0xe3fbe6c0 esi 0xe4098ce8 -469136152 edi 0xe3fbe70c -470030580 eip 0xf6e976e0 0xf6e976e0 eflags 0x10206 [ PF IF RF ] cs 0x23 35 ss 0x2b 43 ds 0x2b 43 es 0x2b 43 fs 0x0 0 gs 0x63 99 (gdb) x/10i $eip => 0xf6e976e0: mov 0x20(%edx),%edx VulnDev reference : vd11006 reported by nils of vulndev ltd. Reproducible: Always

also crashes mac https://crash-stats.mozilla.com/report/index/959f4416-2b3d-4cd3-909a-a7cb62110327 Frame Module Signature [Expand] Source 0 XUL nsLineBox::IndexOf layout/generic/nsIFrame.h:1040 1 XUL nsBlockInFlowLineIterator::nsBlockInFlowLineIterator layout/generic/nsLineBox.h:480 2 XUL nsBlockFrame::ChildIsDirty layout/generic/nsBlockFrame.cpp:5201 3 XUL PresShell::FrameNeedsReflow layout/base/nsPresShell.cpp:3623 4 XUL nsTextFrame::CharacterDataChanged layout/generic/nsTextFrameThebes.cpp:4052 5 XUL nsCSSFrameConstructor::CharacterDataChanged layout/base/nsCSSFrameConstructor.cpp:7940 6 XUL PresShell::CharacterDataChanged layout/base/nsPresShell.cpp:4986 7 XUL nsNodeUtils::CharacterDataChanged content/base/src/nsNodeUtils.cpp:111 8 XUL nsGenericDOMDataNode::SetTextInternal content/base/src/nsGenericDOMDataNode.cpp:398 9 XUL nsQuoteList::RecalcAll layout/base/nsQuoteList.cpp:116 10 XUL nsCSSFrameConstructor::EndUpdate layout/base/nsCSSFrameConstructor.cpp:8315 11 XUL nsDocument::EndUpdate content/base/src/nsDocument.cpp:3999 12 XUL nsHTMLDocument::EndUpdate content/html/document/src/nsHTMLDocument.cpp:2950 13 XUL nsHTMLInputElement::OnValueChanged mozAutoDocUpdate.h:66 .... .... ... also looks like the signature shows up 3 to 4 times a day on a wide range of sites like facebook http://www.pandora.com/ http://www.youtube.com/watch?v=CI3FMj6UW4I&feature=related http://www.softendo.com/game/all/category/Mario+Games+Plat http://www.megaupload.com/?d=0V8UBA7G http://www.globes.co.il/ http://www.radiorock.fi/external/player?vt=audio_stream&vid=52

(with DEBUG_TRACEMALLOC_PRESARENA, and thus useful)

I think the valgrind log shows mainly that the crash is a result of the unbalanced update count that the earlier assertions were reporting.

The interesting parts are: frame #61-#58, where we enter an update on the inner document and then execute script (which basically means we've lost already, as far as I can tell) frame #34-#33, where a flush on the inner document chains up to a flush on the outer document (but script could have happily done that on its own) frame #31, where we enter an update on the outer frame constructor frame #14, in which we destroy the inner frame that we're in an update in from frame #61

(In reply to comment #2) > also looks like the signature shows up 3 to 4 times a day on a wide range of > sites like facebook > http://www.pandora.com/ > http://www.youtube.com/watch?v=CI3FMj6UW4I&feature=related > http://www.softendo.com/game/all/category/Mario+Games+Plat > http://www.megaupload.com/?d=0V8UBA7G > http://www.globes.co.il/ > http://www.radiorock.fi/external/player?vt=audio_stream&vid=52 Please keep stuff you find in crash-stats out of this bug, unless you're confident it's related. I think the odds are it's not.

(In reply to comment #5) > frame #61-#58, where we enter an update on the inner document and then execute > script (which basically means we've lost already, as far as I can tell) By "lost already" I'm speaking in terms of hitting the "dying in the middle of our own update" assertion, which isn't necessarily the main problem, though.

That said, I guess it's the second assertion (and later) that are more important, since they're the ones that are showing unbalanced update counts.

I'm a little stuck on how to continue debugging this.

Based on the crash stack and the nice assertions in comment 0, what happens here is this (with very high probability; I have not verified this in any way, but it's clearly what's happening): 1) BeginUpdate from NodeUtils calls BeginUpdate on the presshell and frame constructor. 2) Pop off removable script blockers, run mutation event, this blows away the presshell and creates a new one, with a new frame constructor. 3) EndUpdate from NodeUtils calls EndUpdate on the new presshell and new frame constructor. Frame constructor asserts and ends up with mUpdateCount == -1. 4) Restyle processing begins and calls BeginUpdate on the frame constructor; mUpdateCount == 0. 5) During restyle processing we destroy a text input's frame. nsTextEditorState::UnbindFromFrame does some stuff inside an update (probably a possible state change notification?) when it stashes away the current value. mUpdateCount goes to 1 while inside this update. 6) The editor update ends, mUpdateCount goes to 0, and the frame constructor rebuilds quotes stuff while the frame tree is in the middle of being destroyed. Then we lose. I see a crash locally (on Linux) on nsIFrame::GetNextSibling where |this| is ARENA_POISON, so it's at least a safe crash. Jonas, I think the only reasonably solution here is that when removable script blockers due to auto updates are popped we need to make the corresponding EndUpdate calls. When they're repushed we need to make the corresponding BeginUpdate calls.

And in particular, we know that it's an UPDATE_CONTENT_MODEL if the script blocker is removable. So all we _really_ need to keep track of is the right document, if any... Jonas, how much pain would it be to keep track of removable script blockers as a stack of nsIDocument pointers as opposed to a counter? The counter would just be the stack length, then. God, I hate mutation events.

Comment 10 makes a lot of sense to me, but I'm not sure what the implications of the suggested solution there would be...

Indeed. More importantly, I'm not sure whether that solution is ok for branches, safety-wise. Might be safer to just ignore EndUpdate on the frame constructor when mUpdateCount == 0 instead.

(In reply to comment #13) > Indeed. More importantly, I'm not sure whether that solution is ok for > branches, safety-wise. Porting that solution to branches really scares me. > Might be safer to just ignore EndUpdate on the frame > constructor when mUpdateCount == 0 instead. Wouldn't that open up a whole new can of worms, though?

> Wouldn't that open up a whole new can of worms, though? Well... like what? And how would it be worse than what we have now, on branches?

(In reply to comment #15) > > Wouldn't that open up a whole new can of worms, though? > > Well... like what? And how would it be worse than what we have now, on > branches? I don't know exactly... is it possible for the changed EndUpdate logic to screw us over in another way, by EndUpdate being ignored when it shouldn't be, for example?

I think we'd want to do whatever EndUpdate does, but just not let the update count go negative.

(In reply to comment #17) > I think we'd want to do whatever EndUpdate does, but just not let the update > count go negative. That should be safer, I think.

that's what I meant in comment 13; I just didn't express it well.

assigning to ehsan (as a guess) per jst

With this fix, the crash doesn't happen, but we get lots of assertions (as expected).

This fix implements the idea in comment 10. Seems to be working fine, and try seems to be happy with it. That said, I'd rather get the branch fix on trunk first, let it bake for a while to make sure that it's safe for branches, then port it to branches, back it out from trunk, and land this fix on trunk. Ideally, it should all happen before the 2.2 cut-off date (April 12th).

Comment on attachment 523507 [details] [diff] [review] Test case (should land with the correct assertion count annotation) Note to self: REFTEST TEST-UNEXPECTED-FAIL | file:///Users/cltbld/talos-slave/test/build/reftest/tests/layout/base/crashtests/645572-1.html | assertion count 42 is more than expected 0 assertions We should probably go with asserts(0-99) on branches, as we don't really care how many assertions this test case generates with the branch fix. It generates no assertions with the trunk fix, as expected.

Comment on attachment 523507 [details] [diff] [review] Test case (should land with the correct assertion count annotation) I bet we could simplify this, but it doesn't seem worthwhile. r=me

Comment on attachment 523567 [details] [diff] [review] Trunk fix Why does testing the total update nesting level against the removable script blocker level make sense?

(In reply to comment #26) > Comment on attachment 523567 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=523567 > Trunk fix > > Why does testing the total update nesting level against the removable script > blocker level make sense? Because we don't want the mUpdateNestLevel value in nsDocuments to be negative.

OK, but we shouldn't call EndUpdate more times than there were _content_ update batches, not total update batches. I think it'd make more sense to explicitly count the UPDATE_CONTENT_MODEL nesting level on the document, expose it via a getter, and just EndUpdate that many times. Another question. Can we have nested removable blockers for different documents? We need to end updates on _all_ documents in this situation, not just the one the removable blocker remover is for.... Jonas? Oh, and the mObserver stuff should be able to go away with the new setup, I think, since we'll now be notifying the sink via the normal path through the document.

Comment on attachment 523567 [details] [diff] [review] Trunk fix r- pending answer to comment 28.

So, in a face-to-face chat, we decided to take the branch fix for 5, and land the more comprehensive ("trunk") fix in 6. Here's the simpler fix for 5: <http://hg.mozilla.org/mozilla-central/rev/0a780111aacb>

We should probably spin the more comprehensive fix into a separate bug....

(In reply to comment #31) > We should probably spin the more comprehensive fix into a separate bug.... Filed bug 650445.

The patches in bug 650493 should provide a better solution than the Trunk fix patch here, right? Could we hold off landing this patch unless it turns out the patches over there won't work for some reason? If so, I'll take this off my review queue.

(In reply to comment #33) > The patches in bug 650493 should provide a better solution than the Trunk fix > patch here, right? I think so. Just to make sure, if you have a build with those patches handy, could you please backout my fix (comment 30) locally and see what happens with the test case? If things are looking good, I'd like to add a patch to bug 650493 which backs out my patch here, since it's not good for us to keep such a badly looking wallpaper too long in the tree.

Yup, the no assertions or crashes with the patches in bug 650493 applied and the patch here backed out. I'll attach a patch there and ask you for review.

Comment on attachment 523508 [details] [diff] [review] Branch fix Approved for 1.9.2.18 and 1.9.1.20, a=dveditz for release-drivers Approved for the mozilla2.0 repository, a=dveditz for release-drivers We only plan on building future 1.9.2 builds so you don't need to land on 1.9.1 or mozilla2.0 unless you want to for consistency.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8f8185f58881 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/eb371eb5778f http://hg.mozilla.org/releases/mozilla-2.0/rev/669cbabc7507

Per security group discussion, requesting landing on mozilla-2.0 (branch fix).

The attached testcase does not crash 1.9.2.17 on XP. Are there STR for 1.9.2.x?

Can I land the testcase for this bug on trunk now?

Crashtest landed: http://hg.mozilla.org/integration/mozilla-inbound/rev/6b728c38e6c6

Crashtest landed on central: http://hg.mozilla.org/mozilla-central/rev/6b728c38e6c6