David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Description • 8 years ago

Split off from bug 1305628. We can immediately begin painting again inside ClientLayerManager's destructor, creating a new compositor and everything. This is a source of potential subtle problems and bug 1305628 isn't the first one that's actually come up.

Matt explained that this is to avoid the nsRefreshDriver getting stuck, so this patch just posts the NotifyTransactionCompleted call to the event loop. It looks like we can probably skip the other DidComposite logic.

	Matt Woodrow (:mattwoodrow)
	Comment 2 • 8 years ago

Comment on attachment 8795447 [details] [diff] [review]
fix

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

::: gfx/layers/client/ClientLayerManager.cpp
@@ +119,1 @@
>      TimeStamp now = TimeStamp::Now();

We can get rid of 'now', right?

	Pulsebot
	Comment 3 • 8 years ago

Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33c254db1f15
Delay the DidComposite call in ClientLayerManager's destructor. (bug 1305829, r=mattwoodrow)

	Carsten Book [:Tomcat]
	Comment 4 • 8 years ago

backed out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=36625827&repo=mozilla-inbound

	Pulsebot
	Comment 5 • 8 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2091e309c88
Backed out changeset 33c254db1f15 for causing memory leaks on OS X

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 6 • 8 years ago

This version has two additional changes. First, the revoke-transaction task is posted in ClientLayerManager::Destroy, rather than the destructor. Second, nsBaseWidget revokes the nsTransactionIdAllocator from the layer manager if the widget is about to shut down.

This should prevent us from reviving the layer manager recursively, and from reviving it during shutdown.

	Pulsebot
	Comment 7 • 8 years ago

Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e4570efaff8
Delay the DidComposite call in ClientLayerManager's destructor. (bug 1305829, r=mattwoodrow)

	Carsten Book [:Tomcat]
	Comment 8 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/7e4570efaff8

	David Anderson [:dvander] - inactive, e-mail if emergency Assignee
	Comment 9 • 8 years ago

Comment on attachment 8812992 [details] [diff] [review]
v2

Approval Request Comment
[Feature/Bug causing the regression]: None
[User impact if declined]: Required to uplift bug 1319213
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Has survived on Nightly. This simply fires a notification earlier than it used to, to avoid a memory leak on shutdown.
[String changes made/needed]: None

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 10 • 8 years ago

Comment on attachment 8812992 [details] [diff] [review]
v2

Fix for memory leak, let's take this on aurora and beta.
It should end up in beta 12, going to build this Thursday.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/52cb6e5dd64d

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 12 • 8 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/743472d4fc92212031be5faeea0ef4ea2c3a9c54

	Marco Castelluccio [:marco]
	Comment 13 • 8 years ago

No crashes on 51.0b12 or 52.0a2 since this landed.