Closed Bug 1036659 Opened 8 years ago Closed 5 years ago

Don't call mozilla::DropJSObjects() in CallbackObject::DropJSObjects()

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mccr8, Assigned: wisniewskit)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

It is safer to only call DropJSObjects() in the dtor, in case of weird future bugs where something gets put into the JS object fields post unlink, or things with subclasses, etc.  DropJSObjects() is not needed in unlink, because it isn't needed to break cycles.  Of course, CallbackObject::DropJSObjects() should get renamed to ClearJSObjects() or something.
Assignee: nobody → continuation
Blocks: 1049167
Unfortunately, just removing the DropJSObjects line in the unlink macro causes crashes on exit, as the debug build's crash trace in this try run shows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3e60a8d2a24

I haven't had a chance to investigate why, and since I'm not familiar with this code I thought I'd at least mention the result.
You need to still clear all the JS values in unlink. And then ensure that mozilla::DropJSObjects(this); is actually called in dtor, whether or not mCallback is null.
So this, basically?

A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e9eef7a3805
Assignee: continuation → wisniewskit
Status: NEW → ASSIGNED
Attachment #8783727 - Flags: review?(bugs)
Comment on attachment 8783727 [details] [diff] [review]
1036659-rename-CallBackObject-DropJSObjects-and-ensure-it-is-called-in-destructor.diff

This doesn't fix the issue mccr8 is worried about. If one sets some js member variable after unlink, ClearJSObjects has been called and so is mozilla::DropJSObjects.

Unlink should just ClearJSReferences and dtor then, if needed, mozilla::DropJSObjects(this);
Attachment #8783727 - Flags: review?(bugs) → review-
Sorry that this one slipped under my radar for so long. Here's a fresh patch that seems to do what you requested.

A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ded92f3f018444c14f86515af56c7901f93edc8
Attachment #8783727 - Attachment is obsolete: true
Attachment #8878721 - Flags: review?(bugs)
Comment on attachment 8878721 [details] [diff] [review]
1036659-dont_call_mozilla::DropJSObjects_in_CallbackObject::DropJSObjects.diff

I'd need to page this stuff in, but mccr8 could do that too :)
(I have couple of other patches to review in my queue)
Attachment #8878721 - Flags: review?(bugs) → review?(continuation)
Comment on attachment 8878721 [details] [diff] [review]
1036659-dont_call_mozilla::DropJSObjects_in_CallbackObject::DropJSObjects.diff

Review of attachment 8878721 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for following up on this.
Attachment #8878721 - Flags: review?(continuation) → review+
Thanks for the review!

Requesting check-in.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8da651d21bb5
Don't call mozilla::DropJSObjects() in CallbackObject::DropJSObjects(). r=mccr8
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8da651d21bb5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.