This bug is about labeling the runnables in nsProxyRelease.{cpp,h}. This could be tricky since it seems like it depends on what we're releasing. Some destructors might touch content and need to be in a certain DocGroup.

I think the first step here is to extend the "hacky patch" of bug 1331804 to NS_ProxyRelease and NS_ReleaseOnMainThread, to figure out what exactly is being proxied so frequently. I think like runnables themselves, these will have to be patched on a case-by-case basis. Then some mechanism will need to be set up to allow that (assuming that these are concentrated enough for it to be worthwhile to label these).

I hacked up a patch to pass through the call site for NS_ProxyRelease and NS_ReleaseOnMainThread, then loaded nytimes.com and scrolled up and down a few times, and this is what it turned up: 158 Running anon_romt(/home/amccreight/mc/image/Decoder.cpp:80) 116 Running anon_romt(/home/amccreight/mc/image/DecodedSurfaceProvider.cpp:52) 2 Running anon_romt(/home/amccreight/mc/image/AnimationSurfaceProvider.cpp:53) So, mostly image lib stuff.

(Those top two call sites are in Decoder::~Decoder and DecodedSurfaceProvider::DropImageReference.)

Is there anything that js/content can observe from these references being dropped? It doesn't seem like it.

It's not uncommon for destructors to end up calling into JS. I don't know if that's the case here, but it can happen in general.

I looked through the RasterImage destructor, everything it calls, and every destructor for fields of RasterImge that would be called, I can't find a path to anything that could be observable from js/content, or run js. The closest I could find is that RasterImage::OnSurfaceDiscard (via removing the image from the surface cache) could be called, which dispatches a runnable that sends a notification.

Part 1 is rather hideously copy-pasta, but it doesn't quite feel like enough to justify yet another layer of templates in that file. The name's not great, but I feel like "system group" is not going to be a concept that is immediately obvious to be mainthread only, so I left the main thread in there. Part 2 just uses the new method, in the two places identified by my analysis, plus a third place in image/ that seems to do the same thing, also with a RasterImage.

Comment on attachment 8832256 [details] Bug 1333974, part 2 - Use new API for images. https://reviewboard.mozilla.org/r/108590/#review109792

(In reply to Andrew McCreight [:mccr8] from comment #10) > Part 2 just uses the new method, in the two places identified by my > analysis, plus a third place in image/ that seems to do the same thing, also > with a RasterImage. Your analysis in comment 2 actually found all three!

Comment on attachment 8832255 [details] Bug 1333974, part 1 - Add new API for releasing on main thread using the system group. https://reviewboard.mozilla.org/r/108588/#review109964 I think it'd be slightly better as `NS_ReleaseOnSystemGroup`; `NS_ReleaseOnMainThreadSystemGroup` makes it sound like there may be other system groups?

(In reply to Nathan Froyd [:froydnj] from comment #13) > I think it'd be slightly better as `NS_ReleaseOnSystemGroup`; > `NS_ReleaseOnMainThreadSystemGroup` makes it sound like there may be other > system groups? Yeah, it is a little weird. My only concern is that it isn't obvious that this implies the main thread. I guess I could explicitly mention that in the comment in case people are confused?

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ecb2dfe369e8 part 1 - Add new API for releasing on main thread using the system group. r=froydnj https://hg.mozilla.org/integration/autoland/rev/4a3433f44ccf part 2 - Use new API for images. r=tnikkel

https://hg.mozilla.org/mozilla-central/rev/ecb2dfe369e8 https://hg.mozilla.org/mozilla-central/rev/4a3433f44ccf