1333974 - Label ProxyRelease runnables for ~Decoder and DecodedSurfaceProvider::DropImageReference

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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).

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Even more hacky labeling, including NS_ProxyRelease and NS_ReleaseOnMainThread. DO NOT LAND

Comment 4

[Andrew McCreight [:mccr8]]

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

Comment 5

[Timothy Nikkel (:tnikkel)]

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

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 7

[Timothy Nikkel (:tnikkel)]

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.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Bug 1333974, part 1 - Add new API for releasing on main thread using the system group.

Review commit: https://reviewboard.mozilla.org/r/108588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108588/

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Bug 1333974, part 2 - Use new API for images.

Review commit: https://reviewboard.mozilla.org/r/108590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108590/

Comment 10

[Andrew McCreight [:mccr8]]

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 11

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8832256 [details]
Bug 1333974, part 2 - Use new API for images.

https://reviewboard.mozilla.org/r/108590/#review109792

Comment 12

[Timothy Nikkel (:tnikkel)]

(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 13

[Nathan Froyd [:froydnj]]

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?

Comment 14

[Andrew McCreight [:mccr8]]

(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?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

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

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