Closed Bug 824868 Opened 7 years ago Closed 7 years ago

AutoLayerManagerSetup doesn't do the right thing if widget has been changed during painting

Categories

(Core :: Widget, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

When the nsAutoScriptBlocker exits in nsViewManager::Refresh we can run script because we add a script runner for the after paint event when we paint (otherwise we shouldn't have anything because we flush before painting in WillPaint). If a script does something that messes with the widget or its layer manager while we have a AutoLayerManagerSetup this can cause problems. For example AutoLayerManagerSetup assumes that GetLayerManager will return the same layer manager before and after the Paint call. And calling AutoLayerManagerSetup at all can create a layer manager, not something we want to do if the widget has had its destructor called.
Attached patch patchSplinter Review
Attachment #695895 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/0bcd652f1732
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 695895 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unsure
User impact if declined: some crashes during painting
Testing completed (on m-c, etc.): on nightly for several days
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Attachment #695895 - Flags: approval-mozilla-aurora?
(In reply to Timothy Nikkel (:tn) from comment #4)
> Comment on attachment 695895 [details] [diff] [review]
> patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): unsure
> User impact if declined: some crashes during painting
> Testing completed (on m-c, etc.): on nightly for several days
> Risk to taking this patch (and alternatives if risky): low risk
> String or UUID changes made by this patch: none

If this is a long standing issue & if the user-scenario is uncommon, could we let this ride the trains instead of uplift here ?
(In reply to bhavana bajaj [:bajaj] from comment #5)
> (In reply to Timothy Nikkel (:tn) from comment #4)
> > Comment on attachment 695895 [details] [diff] [review]
> > patch
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): unsure
> > User impact if declined: some crashes during painting
> > Testing completed (on m-c, etc.): on nightly for several days
> > Risk to taking this patch (and alternatives if risky): low risk
> > String or UUID changes made by this patch: none
> 
> If this is a long standing issue & if the user-scenario is uncommon, could
> we let this ride the trains instead of uplift here ?

nvm,just saw the top-crasher this is blocking (bug 822999).
Comment on attachment 695895 [details] [diff] [review]
patch

Approving on aurora as it helps fix an aurora top crasher(822999)
Attachment #695895 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to bhavana bajaj [:bajaj] from comment #5)
> If this is a long standing issue & if the user-scenario is uncommon, could
> we let this ride the trains instead of uplift here ?

Yeah its a top crasher and I don't seem to be able to find the crashes in the beta crashstats, only aurora and nightly.
You need to log in before you can comment on or make changes to this bug.