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)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

Attachments

(3 files)

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.
Blocks: 795259
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.
(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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Chris - Is this safe enough to uplift to Fx18 (Aurora)? It is likely the #5 crasher on Fx18.
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?
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?
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+
Attachment #673558 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.

Attachment

General

Created:
Updated:
Size: