XPC_WN_Helper_Finalize should use deferred release

RESOLVED FIXED in mozilla15

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: mccr8)

Tracking

Trunk
mozilla15
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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....
(Assignee)

Updated

5 years ago
Assignee: nobody → continuation
(Assignee)

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

Updated

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
(Assignee)

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
(Assignee)

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.

https://tbpl.mozilla.org/?tree=Try&rev=8f08edae73e3
Attachment #624948 - Flags: review?(bobbyholley+bmo)
Comment on attachment 624948 [details] [diff] [review]
more cleanups

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

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4065a1ab0f2b
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/4065a1ab0f2b
Status: NEW → RESOLVED
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.