Last Comment Bug 688044 - ASSERTION: Why is the root in mDirtyRoots already?
: ASSERTION: Why is the root in mDirtyRoots already?
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Mats Palmgren (:mats)
:
:
Mentors:
Depends on:
Blocks: 418511 571959 666306
  Show dependency treegraph
 
Reported: 2011-09-20 15:56 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2011-09-29 01:23 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
462117 (apply first) (7.61 KB, patch)
2011-09-20 15:57 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
666306 (apply second) (41.26 KB, patch)
2011-09-20 15:58 PDT, Jared Wein [:jaws] (please needinfo? me)
no flags Details | Diff | Splinter Review
stack (7.27 KB, text/plain)
2011-09-20 18:58 PDT, Mats Palmgren (:mats)
no flags Details
stack (9.49 KB, text/html)
2011-09-20 22:43 PDT, Mats Palmgren (:mats)
no flags Details
fix (2.14 KB, patch)
2011-09-20 23:25 PDT, Mats Palmgren (:mats)
bzbarsky: review+
Details | Diff | Splinter Review
stack to nsSubDocumentFrame::Reflow (12.23 KB, text/plain)
2011-09-21 14:45 PDT, Mats Palmgren (:mats)
no flags Details

Description Jared Wein [:jaws] (please needinfo? me) 2011-09-20 15:56:57 PDT
With the changes applied from these two patches, this assertion is hit.

To reproduce, apply patch 462117 and then apply patch 666306.
Comment 1 Jared Wein [:jaws] (please needinfo? me) 2011-09-20 15:57:32 PDT
Created attachment 561324 [details] [diff] [review]
462117 (apply first)
Comment 2 Jared Wein [:jaws] (please needinfo? me) 2011-09-20 15:58:04 PDT
Created attachment 561325 [details] [diff] [review]
666306 (apply second)
Comment 3 Boris Zbarsky [:bz] (still a bit busy) 2011-09-20 18:40:42 PDT
What's the concrete class of the root in this case?
Comment 4 Mats Palmgren (:mats) 2011-09-20 18:58:33 PDT
Created attachment 561367 [details]
stack

Here's the stack for when the root frame currently in InitialReflow
is added to 'mDirtyRoots'.  (It's a ViewportFrame)
Comment 5 Mats Palmgren (:mats) 2011-09-20 19:00:13 PDT
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#1951

Maybe InitialReflow should just deal with it?
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-09-20 19:58:16 PDT
Yeah, seems like it....
Comment 7 Mats Palmgren (:mats) 2011-09-20 20:29:09 PDT
Maybe we could do the FrameNeedsReflow stuff before the XBL stuff?
Then it wouldn't add the root frame twice because FrameNeedsReflow bails out of the
ancestor loop on the 'wasDirty' test here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#2840
Comment 8 Mats Palmgren (:mats) 2011-09-20 22:42:03 PDT
Actually, that should just work if the following comment is right...

  // Note: Because the frame just got created, it has the NS_FRAME_IS_DIRTY

but it's not (at that point).  The bit is set when the frame is created but it's
then removed somewhere during ProcessAttachedQueue()...
Comment 9 Mats Palmgren (:mats) 2011-09-20 22:43:48 PDT
Created attachment 561386 [details]
stack

ProcessAttachedQueue runs some script that forces reflow of the root frame.
I'm assuming this is where the bit is removed...
Comment 10 Mats Palmgren (:mats) 2011-09-20 23:25:01 PDT
Created attachment 561394 [details] [diff] [review]
fix

I think we can just skip the FrameNeedsReflow stuff if the bit was cleared.
Try server results pending...
Comment 11 Mats Palmgren (:mats) 2011-09-21 09:28:27 PDT
Comment on attachment 561394 [details] [diff] [review]
fix

Green on Try so far; Android still pending (with a long backlog).
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2011-09-21 13:18:47 PDT
Hmm.  How did that script manage to force reflow of the root frame if the frame already had the dirty bit but wasn't in the reflow root set?
Comment 13 Mats Palmgren (:mats) 2011-09-21 14:45:03 PDT
Created attachment 561574 [details]
stack to nsSubDocumentFrame::Reflow

Here's the stack to nsSubDocumentFrame::Reflow that does the
PostReflowCallback which is what the earlier stack is processing
that leads to the subdocument root frame being reflowed.
(the UpdatePositionAndSize ...)

There are no checks of frame bits or mDirtyRoots on the
UpdatePositionAndSize path,  nsViewManager::DoSetWindowDimensions just calls
ResizeReflow directly.
http://mxr.mozilla.org/mozilla-central/source/view/src/nsViewManager.cpp#261
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-09-21 19:01:45 PDT
Comment on attachment 561394 [details] [diff] [review]
fix

Ah, ok.  This is probably fine, then.

That said, this is a pretty scary change; can you land it after the next aurora branchpoint?
Comment 15 Mozilla RelEng Bot 2011-09-21 20:40:52 PDT
Try run for 2c6e454541bc is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2c6e454541bc
Results (out of 171 total builds):
    success: 160
    warnings: 7
    failure: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-2c6e454541bc
Comment 16 Mozilla RelEng Bot 2011-09-21 21:11:10 PDT
Try run for f2d288a2639a is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f2d288a2639a
Results (out of 171 total builds):
    success: 148
    warnings: 23
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mpalmgren@mozilla.com-f2d288a2639a
Comment 17 Mats Palmgren (:mats) 2011-09-22 11:58:28 PDT
Nothing to worry about in the test failures above... the first one is for
the reviewed patch, with only known Orange.  The second is a different patch.

> this is a pretty scary change; can you land it after the next aurora branchpoint?

OK
Comment 19 Michael Wu [:mwu] 2011-09-29 01:23:43 PDT
https://hg.mozilla.org/mozilla-central/rev/2a89973d7656

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