1325381 - waitUntil() crashes on null-deref when called on script constructed ExtendableEvent()

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

To reproduce, run this in a ServiceWorker script in nightly:

  let e = new ExtendableEvent('foo');
  e.waitUntil(new Promise());

This crashes because mExtensionsHandler is nullptr for script created events, but we try to access it here:

https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp#919

I don't believe this is a sec bug because its a null-deref accessed through a RefPtr<> which does a safe crash.

Catalin, can you take a look at this?  We should fix in FF53 and add a test case for it.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Crash reports:

https://crash-stats.mozilla.com/report/index/a3ee4c10-b818-4f30-8fdf-358742161221
https://crash-stats.mozilla.com/report/index/7f26e8c0-9aeb-485e-88d9-374cf2161222

Not sure I set the crash signature field properly in the bug.

Comment 2

[Cătălin Badea (:catalinb)]

I'll write a patch.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Spec issue to clarify that this sort of thing shouldn't keep the SW alive:

https://github.com/w3c/ServiceWorker/issues/1040

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Sorry, to reproduce crash you need a slightly different example:

```
  let e = new ExtendableEvent('foo');
  e.waitUntil(new Promise(resolve => { }));
```

My previous code was dying on the undefined function passed to the Promise constructor.

Comment 5

[Cătălin Badea (:catalinb)]

Attachment: Fix crash with script constructed ExtendableEvent().waitUntil().

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8821273 [details] [diff] [review]
Fix crash with script constructed ExtendableEvent().waitUntil().

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

Thanks.  I'll mention in the github issue we are going to throw InvalidStateError in this case.  Seems the more conservative thing to do here.

Comment 7

[Andrew Overholt [:overholt]]

Ben, you wanna land this before Catalin gets back?

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I think it can wait until Catalin gets back.

Comment 9

[stefhak]

From my prespective it can wait - I have a workaround in place and watch this bug every now and then and will test without that workaround once it has landed.

Comment 10

[Cătălin Badea (:catalinb)]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/920ef0f4af89
Fix crash with script constructed ExtendableEvent().waitUntil(). r=bkelly

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/920ef0f4af89