1167603 - Mark JSEventHandler::mTarget as MOZ_UNSAFE_REF

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Mark JSEventHandler::mTarget as MOZ_UNSAFE_REF

Comment 2

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

Comment on attachment 8609376 [details] [diff] [review]
Mark JSEventHandler::mTarget as MOZ_UNSAFE_REF

It is no always clear whether to use MOZ_NON_OWNING_REF or MOZ_UNSAFE_REF, but
since MOZ_UNSAFE_REF sounds scarier, it is always fine :)

Comment 3

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

Did you check we actually always call Disconnect() in case someone has set mTarget to non-null value?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

I remember doing some dxr-diving with this bug to try and make sure that the Disconnect handler was actually called, but it may be worth the time to go through the code again (I remember that this one was particularly hairy).

Comment 5

[Nika Layzell [:nika] (ni? for response)]

In https://dxr.mozilla.org/mozilla-central/source/dom/xbl/nsXBLPrototypeHandler.cpp#325, Disconnect is called before the handler, scriptTarget, leaves scope at the end of the method.

The other place where JSEventHandlers are created is https://dxr.mozilla.org/mozilla-central/source/dom/events/EventListenerManager.cpp#621-622. This event handler is associated with a `Listener` object, which ->Disconnect()s the JSEventHandler when it is destroyed. The Listener objects are all destroyed when the mTarget value belonging to the parent EventListenerManager object is invalidated (through EventListenerManager::Disconnect() - which is outside the scope of this patch).

Comment 6

[Carsten Book [:Tomcat]]

can we get a try run for this change ? Thanks!

Comment 7

[Nika Layzell [:nika] (ni? for response)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d42a1d6babfa

Sorry - this change is basically comments (as MOZ_UNSAFE_REF expands to /* nothing */) - so I didn't think I should bother with a try run.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86ed10334619

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86ed10334619

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/2ce3db0db984

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Pulsebot from comment #10)
> https://hg.mozilla.org/integration/b2g-inbound/rev/2ce3db0db984

Ignore this, incorrect bug number.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/86ed10334619