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

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

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.

Comment 1

[Thomas Wisniewski]

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.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 3

[Thomas Wisniewski]

Attachment: 1036659-rename-CallBackObject-DropJSObjects-and-ensure-it-is-called-in-destructor.diff

So this, basically?

A try run seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e9eef7a3805

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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);

Comment 5

[Thomas Wisniewski]

Attachment: 1036659-dont_call_mozilla::DropJSObjects_in_CallbackObject::DropJSObjects.diff

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 7

[Andrew McCreight [:mccr8]]

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.

Comment 8

[Thomas Wisniewski]

Thanks for the review!

Requesting check-in.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8da651d21bb5
Don't call mozilla::DropJSObjects() in CallbackObject::DropJSObjects(). r=mccr8

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8da651d21bb5