634291 - "ASSERTION: Walking off beginning of list" and crash with navigated-away designMode document

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when button is clicked)

###!!! ASSERTION: Walking off beginning of list: 'mChild', file layout/base/nsChildIterator.h, line 114

Crash [@ nsCSSFrameConstructor::FindFrameForContentSibling]

Comment 1

[Jesse Ruderman]

Attachment: stack traces

Comment 2

[Jesse Ruderman]

Crash report: bp-a99c7cb2-d765-47ee-8e47-516032110215

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

nsHTMLEditor is doing things at unsafe time.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

...I think

Comment 5

[(no longer active)]

I don't know the implications of this bug, but noming for blocking to get it on the radar.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

If this gets sg:crit it can block, elsewise: no.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

This is a regression from bug 574529.
It is hard to understand the patch for that bug.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

I think this is what was meant to bug 574529.
Fixes this bug, but I couldn't verify if this regress bug 574529
since that doesn't have a testcase.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 512809 [details] [diff] [review]
patch

> It is hard to understand the patch for that bug.
The point of that bug was to allow ourselves to flush layout in an SVG-as-an-image document *during* (in service to) its host document's paint.  The patch did that by extending the "safe to flush layout" condition to disregard "IsSafeToRunScript" if scripts are disabled.

> since that doesn't have a testcase.
There's no testcase on that bug because it was a helper-bug for implementing SVG-as-an-image.  No standalone testcase was possible.  I'd suggest running reftests on the "reftests/svg/as-image" directory to test this -- in particular, a locally-viewed copy of layout/reftests/svg/as-image/img-and-image-1.html will work this pretty vigorously.

RE the patch here -- it looks good to me!  I wasn't aware of the distinction between GetScriptGlobalObject vs. GetScriptHandlingObject until now, but from reading their documentation in nsIDocument.h, it looks like we do indeed want the latter here.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

layout/reftests/svg/as-image/ reftests work.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 512809 [details] [diff] [review]
patch

r=me

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 512809 [details] [diff] [review]
patch

This should either block or just approved.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 512809 [details] [diff] [review]
patch

a=beltzner

Comment 14

[Andreas Gal :gal]

Does not look exploitable from the stack (near 0 crash).

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Not that particular case, but I believe there a other cases when we shouldn't be
flushing.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/dc1e5032ed96