609821 - "ASSERTION: frame must not be dirty" (nsIFrame::GetCaretBaseline) causing crashtests/420439.html | assertion count 1 is more than expected 0

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires focus)

This just started happening today. I'm guessing it's a regression from http://hg.mozilla.org/mozilla-central/rev/ed0befc22bb7 (bug 389321 part 3), which added GetCaretBaseline.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

###!!! ASSERTION: frame must not be dirty: '!NS_SUBTREE_DIRTY(this)', file layout/generic/nsFrame.cpp, line 909

nsFrame::GetBaseline [layout/generic/nsFrame.cpp:912]
nsIFrame::GetCaretBaseline [layout/generic/nsIFrame.h:987]
nsCaret::GetGeometryForFrame [layout/base/nsCaret.cpp:368]
...

Comment 3

[Timothy Nikkel (:tnikkel)]

I think bug 607857 would probably fix this.

Comment 4

[Phil Ringnalda (:philor)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288947511.1288947890.22494.gz&fulltext=1
Rev3 Fedora 12 mozilla-central debug test crashtest on 2010/11/05 01:58:31
s: talos-r3-fed-007

REFTEST INFO | Loading a blank page
WARNING: NS_ENSURE_TRUE(mDisabledJSAndPlugins) failed: file /builds/slave/mozilla-central-linux-debug/build/editor/composer/src/nsEditingSession.cpp, line 250
###!!! ASSERTION: frame must not be dirty: '!NS_SUBTREE_DIRTY(this)', file /builds/slave/mozilla-central-linux-debug/build/layout/generic/nsFrame.cpp, line 909
nsFrame::GetBaseline [layout/generic/nsFrame.cpp:912]
nsIFrame::GetCaretBaseline [layout/generic/nsIFrame.h:987]
...
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/mozilla-central_fedora-debug_test-crashtest/build/reftest/tests/editor/libeditor/html/crashtests/420439.html | assertion count 1 is more than expected 0 assertions

Comment 5

[(no longer active)]

(In reply to comment #3)
> I think bug 607857 would probably fix this.

I don't think so.  GetPointFromOffset doesn't fail here, so bug 607857 should be a no-op as far as this assertion is concerned.

Comment 6

[(no longer active)]

What's happening here is that nsTableCellFrame doesn't provide its own implementation of GetCaretBaseline, so we fall back to the default implementation in nsIFrame which calls GetBaseline.  Now, nsTableCellFrame doesn't provide GetBaseline either, so we fall back to the nsFrame implementation which asserts that the subtree shouldn't be dirty.

Now, this is a general problem, I think.  PresShell::ProcessReflowCommands calls WillDoReflow, which tries to update the caret position *before* the reflow has happened.  This means that there are probably many cases in which GetBaseline would assert in a similar way.  I'm not sure why we have to update the caret position before reflow, since the reflow is likely to change the caret rect.  Do we have a good reason to do that?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we need to call InvalidateOutsideCaret there, to make sure the right area is invalidated before the caret moves, but we don't need to call (and shoudn't call) UpdateCaretPosition.

Comment 8

[Mats Palmgren (inactive)]

I agree with what roc said above.
FYI, the caret stuff in PresShell::WillDo/DidDoReflow was added in bug 334649.

Comment 9

[Mats Palmgren (inactive)]

BTW, I did submit a patch to TryServer yesterday with that fix and there's
no errors so far.  I also tested a bit manually and it seemed to work fine.

Comment 10

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1289093117.1289093812.11463.gz
OS X 10.5 comm-central-trunk debug test crashtest on 2010/11/06 18:25:17

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1289125630.1289126106.3113.gz
OS X 10.5 comm-central-trunk debug test crashtest on 2010/11/07 02:27:10

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1289134646.1289135437.23565.gz
OS X 10.5 comm-central-trunk debug test crashtest on 2010/11/07 04:57:26

Comment 11

[Legacy TBPL/Treeherder Robot]

philringnalda%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289258281.1289259452.6220.gz
WINNT 5.2 mozilla-central debug test crashtest on 2010/11/08 15:18:01

s: win32-slave23
REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/moz2_slave/mozilla-central-win32-debug-unittest-crashtest/build/reftest/tests/editor/libeditor/html/crashtests/420439.html | assertion count 1 is more than expected 0 assertions

Comment 12

[(no longer active)]

Attachment: Patch (v1)

Comment 13

[Legacy TBPL/Treeherder Robot]

joshmoz%gmail.com
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289314704.1289315612.7632.gz
WINNT 5.2 mozilla-central debug test crashtest on 2010/11/09 06:58:24

s: win32-slave28
REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/moz2_slave/mozilla-central-win32-debug-unittest-crashtest/build/reftest/tests/editor/libeditor/html/crashtests/420439.html | assertion count 1 is more than expected 0 assertions

Comment 14

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/20561a108d5a