714725 - XPC_WN_Helper_Finalize should use deferred release

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

I think it doesn't matter currently, since
only Window wants the Finalize, and it doesn't
return NS_SUCCESS_ALLOW_SLIM_WRAPPERS in PreCreate.
But if some other code starts to use this....

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: cargo culted patch

This is a straightforward bringing over of the code from XPC_WN_NoHelper_Finalize.  Although, if nothing actually uses this code, as Olli said, maybe a better approach would be to just remove that entire clause and assert if there's a slim wrapper.  I think that would just cause a leak, but I'm not sure.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: more cleanups

This version of the patch fixes some additional problems with XPC_WN_Helper_Finalize I noticed.

- XPC_WN_Helper_Finalize doesn't do a null-check before the slim wrapper case, so we'll end up QIing and then releasing a null pointer in that case, so I moved the check before the slim wrapper case.  As with the base bug, apparently this case isn't used in practice right now, so it doesn't matter.
- Bill pointed out that when we finalize we must be in a GC, so we should have an XPCJSRuntime sitting around, so we can eliminate the check and replace it with an assertion.
- Helper_Finalize uses IS_SLIM_WRAPPER instead of IS_SLIM_WRAPPER_OBJECT which I think is unnecessary.
- I replaced a C-style cast with a C++ one.

With these fixes in place, plus the deferred release for slim wrappers, XPC_WN_NoHelper_Finalize and XPC_WN_Helper_Finalize are almost the same, except that in the Helper case we perform a callback.  I factored out the bodies of both functions into a common function WrappedNativeFinalize that takes a flag indicating whether we're in the Helper or NoHelper case.  This should help prevent one version from rotting again.

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 624948 [details] [diff] [review]
more cleanups

Try run looks good-ish.  I think the burning builds are the result of some build infrastructure funkiness.

https://tbpl.mozilla.org/?tree=Try&rev=8f08edae73e3

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 624948 [details] [diff] [review]
more cleanups

Huzzah! :-)

Comment 5

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4065a1ab0f2b

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4065a1ab0f2b