The default bug view has changed. See this FAQ.

XPC_WN_Helper_Finalize should use deferred release

RESOLVED FIXED in mozilla15



5 years ago
5 years ago


(Reporter: smaug, Assigned: mccr8)


Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

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


5 years ago
Assignee: nobody → continuation

Comment 1

5 years ago
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.


5 years ago
OS: Linux → All
Hardware: x86_64 → All
Summary: Should XPC_WN_Helper_Finalize be changed similarly to Bug 712448 ? → XPC_WN_Helper_Finalize should use deferred release
Version: 12 Branch → Trunk

Comment 2

5 years ago
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.
Attachment #624816 - Attachment is obsolete: true

Comment 3

5 years ago
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.
Attachment #624948 - Flags: review?(bobbyholley+bmo)
Comment on attachment 624948 [details] [diff] [review]
more cleanups

Huzzah! :-)
Attachment #624948 - Flags: review?(bobbyholley+bmo) → review+

Comment 5

5 years ago
Target Milestone: --- → mozilla15
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.