Repeated paints in a single transaction fail due to a NULL layer builder pointer

RESOLVED FIXED in Firefox 18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cwiiis, Assigned: cwiiis)

Tracking

Trunk
mozilla19
ARM
Android
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

6 years ago
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)

Updated

6 years ago
Blocks: 795259
(Assignee)

Comment 1

6 years ago
Created attachment 673557 [details] [diff] [review]
Part 1 - Restore old layer builder on return from nsDisplayList::PaintForFrame
Attachment #673557 - Flags: review?(matt.woodrow)
(Assignee)

Comment 2

6 years ago
Created attachment 673558 [details] [diff] [review]
Part 2 - Only remove the layer builder from the created layer manager on destruction of a ClippedDisplayItem
Attachment #673558 - Flags: review?(matt.woodrow)
(Assignee)

Comment 3

6 years ago
Created attachment 673559 [details] [diff] [review]
Part 3 - Assert the layer builder exists in FrameLayerBuilder::DrawThebesLayer
Attachment #673559 - Flags: review?(matt.woodrow)
Yay, I've been running into this but I haven't debugged it yet. That's a real life saver :)
Attachment #673557 - Flags: review?(matt.woodrow) → review+
Attachment #673559 - Flags: review?(matt.woodrow) → review+
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

6 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 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+
Do you want me to land these changes now Cwiiis? It's a nasty bug for progressive drawing.

Updated

6 years ago
Duplicate of this bug: 803262
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
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Duplicate of this bug: 798863
Chris - Is this safe enough to uplift to Fx18 (Aurora)? It is likely the #5 crasher on Fx18.
status-firefox18: --- → affected
status-firefox19: --- → fixed
(Assignee)

Comment 14

6 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

6 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

6 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 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

6 years ago
Attachment #673558 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

6 years ago
Attachment #673559 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.