918450 - Crash in dispatching mozilla::dom::workers::EventListenerManager::DispatchEvent

Status: VERIFIED FIXED

(Core :: DOM: Workers, defect)

Description

[(no longer active)]

This is reproducible on trunk, just load the URL.

https://crash-stats.mozilla.com/report/index/bc0b2a65-c497-4705-895f-452a42130919
https://crash-stats.mozilla.com/report/index/ad03a02e-673f-473d-9614-83d0d2130919

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ehsan, thank you!  This is almost certainly the same thing as bug 916860... but I can reproduce this one.

So we land in mozilla::dom::workers::EventListenerManager::DispatchEvent and on this line:

390         if (!JS_ValueToObject(aCx, listenerVal, listenerObj.address())) {

here listenerVal is dead:

(gdb) p listenerVal.toObject().slots
$8 = ('js::HeapSlot' *) 0xdadadadadadadada
(gdb) p *listenerVal.toObject().getClass()
$10 = {
  name = 0x144a92b80 "@,�D\001", 
  flags = 1151937408, 
  addProperty = 0xdadadadadadadada, 
  delProperty = 0xdadadadadadadada, 
  getProperty = 0xdadadadadadadada, 

etc.

Comment 2

[Boris Zbarsky [:bzbarsky]]

And, unsurprisingly, collection->mListeners.getFirst()->mListener.ptr is the same dead object.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I can reproduce this bug even if I back out bug 914334.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So fwiw, if I build rev e5ca10a2b3d0 (the start of the range in bug 916860) this bug goes away. So bisecting now.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The first bad revision is:
changeset:   146382:f4804e82856f
user:        Andrew McCreight <[e-mail]>
date:        Tue Sep 10 08:29:44 2013 -0700
summary:     Bug 911333 - Remove customTrace from bindings codegen. r=bz

I suck.  We thought this was unused, but this part is key:

-        self.customTrace = desc.get('customTrace', self.workers)

Comment 6

[Andrew McCreight [:mccr8]]

D'oh.  I noticed that bit of code and somehow didn't put it together.  I think the right thing to do is to just back out bug 911333 from Aurora and Nightly...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Yep. Verifying that the backout fixes the problem.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Back out the fix for bug 911333, since trace hooks are in fact used in workers.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 807561 [details] [diff] [review]
Back out the fix for bug 911333, since trace hooks are in fact used in workers.

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

Clearly we need a test for this too.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 807561 [details] [diff] [review]
Back out the fix for bug 911333, since trace hooks are in fact used in workers.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not sure.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  Not quite.

Which older supported branches are affected by this flaw?  Aurora 26.

If not all supported branches, which bug introduced the flaw?  Bug 911333.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  This patch should apply to Aurora 26.

How likely is this patch to cause regressions; how much testing does it need?  Not likely to cause problems.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 807561 [details] [diff] [review]
Back out the fix for bug 911333, since trace hooks are in fact used in workers.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 911333.
User impact if declined: Crashes in worker code.
Testing completed (on m-c, etc.): Fixes the testcase here.
Risk to taking this patch (and alternatives if risky): Very low risk
String or IDL/UUID changes made by this patch: None.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 807561 [details] [diff] [review]
Back out the fix for bug 911333, since trace hooks are in fact used in workers.

Sec-approval+ for trunk and approval for Aurora after it goes into trunk and things are green.

Comment 13

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8eb674a22a17

Comment 14

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

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cf30adef42d3

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Should bug 911333 be marked WONTFIX?

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> Should bug 911333 be marked WONTFIX?

No, I can still do a more limited version.  I'll probably make the custom trace hook thing worker-only.  I also need to investigate what workers are doing with them, as that isn't really the CC way of doing things.

Comment 18

[Tracy Walker [:tracy]]

This signature is no longer present on Fx26 and Fx27