"ASSERTION: We shouldn't be reentering here" involving scrollbar reflow and editor

RESOLVED FIXED in mozilla2.0b4

Status

()

defect
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: jruderman, Assigned: bzbarsky)

Tracking

(Blocks 1 bug, {assertion})

Trunk
mozilla2.0b4
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Posted file stack trace
###!!! ASSERTION: We shouldn't be reentering here: '!mFrameIsUpdatingScrollbar', file /home/jruderman/mozilla-central/layout/generic/nsGfxScrollFrame.cpp, line 2981

I couldn't figure out how to reproduce this assertion, but the stack trace looks useful.  Other bugs suspicious of nsGfxScrollFrameInner::SetCoordAttribute include bug 373564 (which smaug fixed in 2007), bug 437537, and bug 338674.
This looks related to bug 471166 to me.

One way to fix this would be to set a script blocker in nsGfxScrollFrameInner::SetCoordAttribute, but I wonder if a better fix would be to always run EditingStateChanged off of a script runner?
This just happened on Tinderbox.
OK, looks like this blocks relanding bug 577607.

I think I've convinced myself that the editor thing is the only issue here.

What's happening is that when we get our contenteditable element initially, we bail out of nsHTMLDocument::DeferredContentEditableCountChange because mParser is non-null.  In EndLoad we null out the parser and call EditingStateChanged, but HasPresShell() returns false for the window because we haven't done the async frame init in the parent yet as far as I can tell.  So we skip out of EditingStateChanged and leave a guerilla editor init lying around to run the next time an EndUpdate happens to come along while we have a presshell.
Blocks: 577607
Status: UNCONFIRMED → NEW
Ever confirmed: true
Posted patch Proposed fix (obsolete) — Splinter Review
Attachment #465075 - Flags: review?(roc)
Posted patch Somewhat safer (obsolete) — Splinter Review
I tested the patch some, and realized that it can cause crashes because CreateShell is not called inside a scriptblocker (and that in general, document viewer init is not all that robust).

So I modified it such that if MaybeEditingStateChanged modifies the mPresShell of the document from whatever doCreateShell set up we bail out.  I think that's the safe thing to do for now, pending more sanity in editor-land and document viewer.
Attachment #465075 - Attachment is obsolete: true
Except that double-deletes the styleset...  We really need a scriptblocker here.
For what it's worth, I tried a scriptblocker.  Then we crash in nsFrameLoader::Show because showing the viewer makes its mDocShell null when running docshell/base/crashtests/432114-1.html.

Then I tried null-checking that and end up with nasty docloader and necko asserts.
Attachment #465292 - Attachment is obsolete: true
Attachment #465348 - Flags: review?(roc)
Attachment #465348 - Flags: approval2.0?
Comment on attachment 465348 [details] [diff] [review]
OK, this seems like what I want to do here

r=dbaron
Attachment #465348 - Flags: review?(roc)
Attachment #465348 - Flags: review+
Attachment #465348 - Flags: approval2.0?
Attachment #465348 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/3dd0ae211428
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Assignee: nobody → bzbarsky
Flags: in-testsuite+
Priority: -- → P1
Target Milestone: --- → mozilla2.0b4
You need to log in before you can comment on or make changes to this bug.