1126739 - Add locking to SurfaceCache entry points that bypass the public API

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

I messed up in bug 1116733. =( Although I added locking to all of SurfaceCache's public API entry points, I failed to add locking to some entry points that bypass the public API. These are:

- SurfaceTracker::NotifyExpired()
- MemoryPressureObserver::Observe()
- SurfaceCache::CollectReports()

This is almost certainly responsible for bug 1122744, and probably other failures as well.

This will need uplift to Aurora.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Add locking to SurfaceCache entry points that bypass the public API

Here's the patch.

I made a tweak to SurfaceTracker to make it look more like all the other sites
where we do locking, because consistency is wise with this kind of thing.

Comment 2

[Nathan Froyd [:froydnj]]

Assuming I've got my releases correct...

[Tracking Requested - why for this release]: Crashes, probably responsible for bug 1122744, intermittent failures in automation, and likely other things as well.

Comment 3

[Seth Fowler [:seth] [:s2h]]

[Tracking Requested - why for this release]: Same as in comment 2, but this is on Aurora also.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Pushed despite inbound being closed so this can make the next Nightly:

https://hg.mozilla.org/integration/mozilla-inbound/rev/eedc4452c4c3

Comment 5

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8555794 [details] [diff] [review]
Add locking to SurfaceCache entry points that bypass the public API

Approval Request Comment
[Feature/regressing bug #]: 1116733
[User impact if declined]: Crashes and erratic behavior related to images. Also intermittent oranges due to image surfaces being released when they should have been locked.
[Describe test coverage new/current, TreeHerder]: Green on try. (And pushed to inbound but not yet merged to central.)
[Risks and why]: This is really low risk - just adds locking in some places that were missed in bug 1116733.
[String/UUID change made/needed]: None.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b27289131320 for static analysis and mochitest-4 bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=6045886&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=6046328&repo=mozilla-inbound

Comment 7

[Seth Fowler [:seth] [:s2h]]

Attachment: Add locking to SurfaceCache entry points that bypass the public API

Both of those issues were super quick fixes. I just ran through the image tests
locally to make sure nothing else got missed.

I'll submit a new try job too (I thought I submitted a try job for this before, but I can't find it!) and will repush after that's confirmed green.

Comment 8

[Seth Fowler [:seth] [:s2h]]

OK, here's the try job:

https://tbpl.mozilla.org/?tree=Try&rev=5d602f3fa506

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: Add locking to SurfaceCache entry points that bypass the public API

browser_bug666317.js failed because it expected DISCARD to be delivered
synchronously, but we made it async. It's an easy fix: we just need to pump the
event loop to allow the event to be delivered.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Should be ready to go now. Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c85f765f2d

Comment 11

[Carsten Book [:Tomcat]]

(In reply to Seth Fowler [:seth] from comment #10)
> Should be ready to go now. Pushed:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c85f765f2d

sorry but this turned out to be still failing and so i had to back this out. Test failure was https://treeherder.mozilla.org/logviewer.html#?job_id=6059522&repo=mozilla-inbound

Comment 12

[Seth Fowler [:seth] [:s2h]]

Attachment: Add locking to SurfaceCache entry points that bypass the public API

Sigh. OK, apparently setTimeout(..., 0) is not a safe way to pump the event
queue, because setTimeout functions run when the next timer event fires, which
might've already been in the event queue before the DISCARD event got put there.
I talked to bz about this and he suggested using window.postMessage instead, so
this new version of the patch uses that approach. It works locally, but then, so
did the old approach, so I've pushed a new bc-only run on all desktop OSes:

https://tbpl.mozilla.org/?tree=Try&rev=0a1e6f6b9b5a

Comment 13

[Seth Fowler [:seth] [:s2h]]

I talked with KWierso and we concluded that the test failure in the above try push is probably unrelated. Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f274f6a5616f

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f274f6a5616f

Comment 15

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8556636 [details] [diff] [review]
Add locking to SurfaceCache entry points that bypass the public API

Approval Request Comment
[Feature/regressing bug #]: 1116733
[User impact if declined]: Crashes and erratic behavior related to images. Also intermittent oranges due to image surfaces being released when they should have been locked.
[Describe test coverage new/current, TreeHerder]: On central.
[Risks and why]: This is really low risk - just adds locking in some places that were missed in bug 1116733. (Some tests had to be updated because a notification was changed from sync to async, but that notification is _only_ used in tests, so this change will not affect users.)
[String/UUID change made/needed]: None.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8556636 [details] [diff] [review]
Add locking to SurfaceCache entry points that bypass the public API

Glad to see this patch stick on m-c. Let's get it on Aurora. Aurora+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c63448c2184