Last Comment Bug 645572 - Crash in xul!nsBlockInFlowLineIterator::nsBlockInFlowLineIterator [@ nsLineBox::IndexOf ] [@ nsLineBox::IndexOf(nsIFrame*) ]
: Crash in xul!nsBlockInFlowLineIterator::nsBlockInFlowLineIterator [@ nsLine...
Status: RESOLVED FIXED
[sg:critical?] [qa-examined-192] [qa-...
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- critical (vote)
: mozilla5
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on:
Blocks: 570215
  Show dependency treegraph
 
Reported: 2011-03-27 16:21 PDT by Nils
Modified: 2011-09-09 07:42 PDT (History)
8 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
.x-fixed
.18+
.18-fixed
wontfix


Attachments
testcase (crashes browser) (2.11 KB, text/html)
2011-03-27 16:22 PDT, Nils
no flags Details
valgrind output (48.02 KB, text/plain; charset=UTF-8)
2011-03-27 19:42 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
stack of first assertion (17.57 KB, text/plain; charset=UTF-8)
2011-03-27 20:18 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
Test case (should land with the correct assertion count annotation) (3.28 KB, patch)
2011-03-31 20:10 PDT, :Ehsan Akhgari
bzbarsky: review+
Details | Diff | Splinter Review
Branch fix (948 bytes, patch)
2011-03-31 20:12 PDT, :Ehsan Akhgari
jonas: review+
dveditz: approval2.0+
dveditz: approval1.9.2.18+
dveditz: approval1.9.1.20+
Details | Diff | Splinter Review
Trunk fix (7.22 KB, patch)
2011-04-01 06:23 PDT, :Ehsan Akhgari
bzbarsky: review-
Details | Diff | Splinter Review

Description Nils 2011-03-27 16:21:40 PDT
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
Comment 1 Nils 2011-03-27 16:22:40 PDT
Created attachment 522259 [details]
testcase (crashes browser)
Comment 2 chris hofmann 2011-03-27 19:32:22 PDT
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
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-27 19:42:38 PDT
Created attachment 522269 [details]
valgrind output

(with DEBUG_TRACEMALLOC_PRESARENA, and thus useful)
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-27 19:46:13 PDT
I think the valgrind log shows mainly that the crash is a result of the unbalanced update count that the earlier assertions were reporting.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-27 20:18:44 PDT
Created attachment 522276 [details]
stack of first assertion

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
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-27 20:19:53 PDT
(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.
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-27 20:21:46 PDT
(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.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-27 20:29:43 PDT
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.
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-03-27 20:35:12 PDT
I'm a little stuck on how to continue debugging this.
Comment 10 Boris Zbarsky [:bz] 2011-03-27 20:38:46 PDT
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.
Comment 11 Boris Zbarsky [:bz] 2011-03-27 20:41:33 PDT
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 12 :Ehsan Akhgari 2011-03-28 12:06:34 PDT
Comment 10 makes a lot of sense to me, but I'm not sure what the implications of the suggested solution there would be...
Comment 13 Boris Zbarsky [:bz] 2011-03-28 12:29:59 PDT
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.
Comment 14 :Ehsan Akhgari 2011-03-28 13:39:29 PDT
(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?
Comment 15 Boris Zbarsky [:bz] 2011-03-28 17:41:39 PDT
> 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?
Comment 16 :Ehsan Akhgari 2011-03-29 10:43:04 PDT
(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?
Comment 17 Boris Zbarsky [:bz] 2011-03-29 10:47:04 PDT
I think we'd want to do whatever EndUpdate does, but just not let the update count go negative.
Comment 18 :Ehsan Akhgari 2011-03-29 17:31:06 PDT
(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.
Comment 19 Boris Zbarsky [:bz] 2011-03-29 18:14:19 PDT
that's what I meant in comment 13; I just didn't express it well.
Comment 20 Daniel Veditz [:dveditz] 2011-03-31 13:43:32 PDT
assigning to ehsan (as a guess) per jst
Comment 21 :Ehsan Akhgari 2011-03-31 20:10:45 PDT
Created attachment 523507 [details] [diff] [review]
Test case (should land with the correct assertion count annotation)
Comment 22 :Ehsan Akhgari 2011-03-31 20:12:50 PDT
Created attachment 523508 [details] [diff] [review]
Branch fix

With this fix, the crash doesn't happen, but we get lots of assertions (as expected).
Comment 23 :Ehsan Akhgari 2011-04-01 06:23:31 PDT
Created attachment 523567 [details] [diff] [review]
Trunk fix

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 24 :Ehsan Akhgari 2011-04-01 06:25:37 PDT
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 25 Boris Zbarsky [:bz] 2011-04-01 12:09:02 PDT
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 26 Boris Zbarsky [:bz] 2011-04-01 12:14:22 PDT
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?
Comment 27 :Ehsan Akhgari 2011-04-01 14:27:41 PDT
(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.
Comment 28 Boris Zbarsky [:bz] 2011-04-01 19:58:30 PDT
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 29 Boris Zbarsky [:bz] 2011-04-06 23:44:10 PDT
Comment on attachment 523567 [details] [diff] [review]
Trunk fix

r- pending answer to comment 28.
Comment 30 :Ehsan Akhgari 2011-04-08 14:34:33 PDT
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>
Comment 31 Boris Zbarsky [:bz] 2011-04-08 14:40:32 PDT
We should probably spin the more comprehensive fix into a separate bug....
Comment 32 :Ehsan Akhgari 2011-04-15 18:24:06 PDT
(In reply to comment #31)
> We should probably spin the more comprehensive fix into a separate bug....

Filed bug 650445.
Comment 33 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-18 19:56:36 PDT
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.
Comment 34 :Ehsan Akhgari 2011-04-18 21:05:21 PDT
(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.
Comment 35 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-04-19 15:09:35 PDT
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 36 Daniel Veditz [:dveditz] 2011-05-13 10:58:50 PDT
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.
Comment 38 Cameron Kaiser [:spectre] 2011-05-21 16:34:47 PDT
Per security group discussion, requesting landing on mozilla-2.0 (branch fix).
Comment 39 Al Billings [:abillings] 2011-06-14 13:51:54 PDT
The attached testcase does not crash 1.9.2.17 on XP. Are there STR for 1.9.2.x?
Comment 40 :Ehsan Akhgari 2011-06-28 14:59:46 PDT
Can I land the testcase for this bug on trunk now?
Comment 41 :Ehsan Akhgari 2011-09-08 15:11:38 PDT
Crashtest landed: http://hg.mozilla.org/integration/mozilla-inbound/rev/6b728c38e6c6
Comment 42 :Ehsan Akhgari 2011-09-09 07:42:38 PDT
Crashtest landed on central: http://hg.mozilla.org/mozilla-central/rev/6b728c38e6c6

Note You need to log in before you can comment on or make changes to this bug.