Last Comment Bug 714725 - XPC_WN_Helper_Finalize should use deferred release
: XPC_WN_Helper_Finalize should use deferred release
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Andrew McCreight (PTO-ish through 6-29) [:mccr8]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-03 02:06 PST by Olli Pettay [:smaug] (high review load, please consider other reviewers)
Modified: 2012-05-19 18:38 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-03 02:06:32 PST
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 (PTO-ish through 6-29) [: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 Andrew McCreight (PTO-ish through 6-29) [: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 Andrew McCreight (PTO-ish through 6-29) [: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.

https://tbpl.mozilla.org/?tree=Try&rev=8f08edae73e3
Comment 4 Bobby Holley (busy) 2012-05-18 10:32:18 PDT
Comment on attachment 624948 [details] [diff] [review]
more cleanups

Huzzah! :-)
Comment 5 Andrew McCreight (PTO-ish through 6-29) [:mccr8] 2012-05-19 13:59:12 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4065a1ab0f2b
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-05-19 18:38:00 PDT
https://hg.mozilla.org/mozilla-central/rev/4065a1ab0f2b

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