Don't create checkpoints while painting with AutoAssertNoContentJS on the stack

RESOLVED FIXED in Firefox 65

Status

()

defect
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

Posted patch patchSplinter Review
Currently, Web Replay checkpoints are created after each paint as part of the NotifyPaintStart call.  This is problematic because creating a checkpoint can cause content JS to run --- for example, the debugger could trigger a synchronous paint, which runs content JS via nsRefreshDriver::RunFrameRequestCallbacks --- and content JS may be prohibited by the calling PresShell::Paint frame.

The attached patch resolves this by moving the CreateCheckpoint call so that it happens in PresShell::Paint when content JS is allowed again.
Attachment #9023846 - Flags: review?(nical.bugzilla)
Comment on attachment 9023846 [details] [diff] [review]
patch

Review of attachment 9023846 [details] [diff] [review]:
-----------------------------------------------------------------

I never touched the innards of PresShell.cpp so I would rather transfer the review to someone from the layout team.
Attachment #9023846 - Flags: review?(nical.bugzilla) → review?(mstange)
Comment on attachment 9023846 [details] [diff] [review]
patch

Review of attachment 9023846 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, moving this call from NotifyPaintStart (which runs during ShadowLayerForwarder::EndTransaction) to PresShell::Paint seems fine to me. It would be nicer to move it even more up the call stack, but I can't see a good place for that, so I think this is good the way it is.
Attachment #9023846 - Flags: review?(mstange) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc6ffce8b9b6
Don't create checkpoints while painting with AutoAssertNoContentJS on the stack, r=mstange.
https://hg.mozilla.org/mozilla-central/rev/bc6ffce8b9b6
Status: NEW → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.