Last Comment Bug 714725 - XPC_WN_Helper_Finalize should use deferred release
: XPC_WN_Helper_Finalize should use deferred release
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla15
Assigned To: Andrew McCreight [:mccr8]
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2012-01-03 02:06 PST by Olli Pettay [:smaug] (pto-ish for couple of days)
Modified: 2012-05-19 18:38 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

cargo culted patch (1.20 KB, patch)
2012-05-17 11:55 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
more cleanups (3.45 KB, patch)
2012-05-17 16:25 PDT, Andrew McCreight [:mccr8]
bobbyholley: review+
Details | Diff | Splinter Review

Description User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-01-03 02:06:32 PST
I think it doesn't matter currently, since
only Window wants the Finalize, and it doesn't
But if some other code starts to use this....
Comment 1 User image Andrew McCreight [:mccr8] 2012-05-17 11:55:24 PDT
Created attachment 624816 [details] [diff] [review]
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 User image Andrew McCreight [:mccr8] 2012-05-17 16:25:18 PDT
Created attachment 624948 [details] [diff] [review]
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 User image Andrew McCreight [:mccr8] 2012-05-18 10:21:34 PDT
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.
Comment 4 User image Bobby Holley (:bholley) (busy with Stylo) 2012-05-18 10:32:18 PDT
Comment on attachment 624948 [details] [diff] [review]
more cleanups

Huzzah! :-)
Comment 5 User image Andrew McCreight [:mccr8] 2012-05-19 13:59:12 PDT
Comment 6 User image Ryan VanderMeulen [:RyanVM] 2012-05-19 18:38:00 PDT

Note You need to log in before you can comment on or make changes to this bug.