Closed
Bug 803826
Opened 12 years ago
Closed 12 years ago
Repeated paints in a single transaction fail due to a NULL layer builder pointer
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: cwiiis, Assigned: cwiiis)
References
Details
Attachments
(3 files)
2.73 KB,
patch
|
mattwoodrow
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
5.69 KB,
patch
|
mattwoodrow
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
mattwoodrow
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
With progressive tile painting turned on, there are relatively rare, but repeatable crashes. Stacks show that FrameLayerBuilder::DrawThebesLayer is calling CheckDOMModified with a NULL layer builder.
This happens because in a couple of places, it's possible for the layer builder user data on the layer manager to be reset to NULL before the layer transaction has ended.
Patches incoming.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #673557 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #673558 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #673559 -
Flags: review?(matt.woodrow)
Comment 4•12 years ago
|
||
Yay, I've been running into this but I haven't debugged it yet. That's a real life saver :)
Updated•12 years ago
|
Attachment #673557 -
Flags: review?(matt.woodrow) → review+
Updated•12 years ago
|
Attachment #673559 -
Flags: review?(matt.woodrow) → review+
Comment 5•12 years ago
|
||
Comment on attachment 673558 [details] [diff] [review]
Part 2 - Only remove the layer builder from the created layer manager on destruction of a ClippedDisplayItem
Review of attachment 673558 [details] [diff] [review]:
-----------------------------------------------------------------
::: layout/base/FrameLayerBuilder.cpp
@@ +2485,5 @@
> }
>
> FrameLayerBuilder::ClippedDisplayItem::~ClippedDisplayItem()
> {
> + if (mInactiveLayerManager && mOwnsLayerManager) {
Why are we only removing the reference to the FrameLayerBuilder if mOwnsLayerManager?
The reference gets added in AddLayerDisplayItem (regardless of whether 'createdManager' was true/false), and we need to make sure that we always remove it again at the end of the paint.
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Comment on attachment 673558 [details] [diff] [review]
> Part 2 - Only remove the layer builder from the created layer manager on
> destruction of a ClippedDisplayItem
>
> Review of attachment 673558 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +2485,5 @@
> > }
> >
> > FrameLayerBuilder::ClippedDisplayItem::~ClippedDisplayItem()
> > {
> > + if (mInactiveLayerManager && mOwnsLayerManager) {
>
> Why are we only removing the reference to the FrameLayerBuilder if
> mOwnsLayerManager?
>
> The reference gets added in AddLayerDisplayItem (regardless of whether
> 'createdManager' was true/false), and we need to make sure that we always
> remove it again at the end of the paint.
You're right, didn't read this correctly - I can remove this check, it's not important as I assume that once items are being torn down, the transaction is over anyway(?)
Is this an r+ with this part of the change removed?
Comment 7•12 years ago
|
||
Comment on attachment 673558 [details] [diff] [review]
Part 2 - Only remove the layer builder from the created layer manager on destruction of a ClippedDisplayItem
Review of attachment 673558 [details] [diff] [review]:
-----------------------------------------------------------------
Yep, remove all the mOwnsLayerManager/createdManager bits and this is fine with me.
Attachment #673558 -
Flags: review?(matt.woodrow) → review+
Comment 8•12 years ago
|
||
Do you want me to land these changes now Cwiiis? It's a nasty bug for progressive drawing.
Assignee | ||
Comment 9•12 years ago
|
||
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6b9d82bf9a94
https://hg.mozilla.org/mozilla-central/rev/51c21a53a849
https://hg.mozilla.org/mozilla-central/rev/edd0d1d9cd0b
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 13•12 years ago
|
||
Chris - Is this safe enough to uplift to Fx18 (Aurora)? It is likely the #5 crasher on Fx18.
Updated•12 years ago
|
status-firefox18:
--- → affected
status-firefox19:
--- → fixed
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 673557 [details] [diff] [review]
Part 1 - Restore old layer builder on return from nsDisplayList::PaintForFrame
[Approval Request Comment]
Bug caused by (feature/regressing bug #): DLBI, I suppose
User impact if declined: Infrequent crashes
Testing completed (on m-c, etc.): Been on m-c for a couple of days
Risk to taking this patch (and alternatives if risky): Low risk, I believe.
String or UUID changes made by this patch: None
Attachment #673557 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 15•12 years ago
|
||
Comment on attachment 673558 [details] [diff] [review]
Part 2 - Only remove the layer builder from the created layer manager on destruction of a ClippedDisplayItem
[Approval Request Comment]
Bug caused by (feature/regressing bug #): DLBI, I suppose
User impact if declined: Infrequent crashes
Testing completed (on m-c, etc.): Been on m-c for a couple of days
Risk to taking this patch (and alternatives if risky): Low risk, I believe.
String or UUID changes made by this patch: None
Will commit the updated version of the patch that landed on m-c.
Attachment #673558 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 673559 [details] [diff] [review]
Part 3 - Assert the layer builder exists in FrameLayerBuilder::DrawThebesLayer
[Approval Request Comment]
Bug caused by (feature/regressing bug #): DLBI, I suppose
User impact if declined: No assertion in a situation that would cause crashes
Testing completed (on m-c, etc.): Been on m-c for a couple of days
Risk to taking this patch (and alternatives if risky): No risk, just adds an assertion
String or UUID changes made by this patch: None
Attachment #673559 -
Flags: approval-mozilla-aurora?
Comment 17•12 years ago
|
||
Comment on attachment 673557 [details] [diff] [review]
Part 1 - Restore old layer builder on return from nsDisplayList::PaintForFrame
Approving as it fixes a top crasher and low risk . Please let us know if we need to do specific QAing or be on lookout for some kind of regressions
Attachment #673557 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #673558 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•12 years ago
|
Attachment #673559 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 18•12 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•