1347739 - ExtendableMessageEvent `source` accessor crashes tab

Status: RESOLVED FIXED

Description

[Mike Pennisi [:jugglinmike]]

Accessing the `source` attribute of a user-created `ExtendableMessageEvent`
object causes the browser tab of the current client to crash.

Steps to reproduce:

1. Execute the following code in the global scope of a service worker:

       new ExtendableMessageEvent('type').source;

Expected result: the expression should evaluate to `null` [1]

Actual result: the browser tab crashes

This appears to originate from the accessor function itself (rather than the
property access). The following code does *not* cause a crash:

    var e = new ExtendableMessageEvent('type');
    var desc = Object.getOwnPropertyDescriptor(ExtendableMessageEvent.prototype, 'source');
    var getter = desc.get;

However, invoking the accessor directly also causes a crash:

    getter.call(e);

No crash occurs for `ExtendableMessageEvent` objects that have been constructed
internally, i.e. for `message` event handlers.

The only open bug report I can find for the `source` attribute is bug 1196097,
but that does not seem related to this issue.

Operating System: Ubuntu 16.04 (x86_64)
Browser: Firefox Nightly
Contents of `platform.ini` file:
    BuildID=20170314110401
    Milestone=55.0a1
    SourceRepository=https://hg.mozilla.org/mozilla-central
    SourceStamp=6d38ad302429c98115c354d643e81987ecec5d3c

[1] https://w3c.github.io/ServiceWorker/#extendablemessage-event-source

> 4.8.4. `event.source`
>
> The `source` attribute must return the value it was initialized to. When the
> object is created, this attribute must be initialized to null. It represents
> the Client object from which the message is sent.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

We're hitting this MOZ_CRASH:

https://hg.mozilla.org/mozilla-central/annotate/8dd496fd015a/dom/workers/ServiceWorkerEvents.cpp#l1194

I guess we should just return nullptr instead.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Mike, do you have a test for this already written?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Don't crash if an ExtendableMessageEvent object has a null source. r=asuth

This fixes the crash locally for me.  I'll wait for Mike to respond before writing a test.  If he has one I can either import it or just wait for it to merge from upstream.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I also checked MessageEvent.  It does not have this problem:

https://dxr.mozilla.org/mozilla-central/source/dom/events/MessageEvent.cpp#90

Comment 5

[Mike Pennisi [:jugglinmike]]

(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #2)
> Mike, do you have a test for this already written?

Sure do--it's on its way to Web Platform Tests via the Chromium project's automated "upstreaming" process. Currently under review at https://codereview.chromium.org/2751113005/

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Ok.  I'm just going to land this then.  We'll get the test when it merges around.  Thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b0e80d2bd3
Don't crash if an ExtendableMessageEvent object has a null source. r=asuth

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

We might as well uplift this to beta/aurora since its such an easy fix.

Comment 9

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/77b0e80d2bd3

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8848073 [details] [diff] [review]
Don't crash if an ExtendableMessageEvent object has a null source. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: Crash if certain uncommon javascript is execute by a site.
[Is this code covered by automated tests?]: WPT tests are upstream, but not in our tree yet.
[Has the fix been verified in Nightly?]: I verified the fix locally.
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal risk.
[Why is the change risky/not risky?]: This basically removes an incorrect explicit MOZ_CRASH() from the code.  No other logic was changed.
[String changes made/needed]: None

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8848073 [details] [diff] [review]
Don't crash if an ExtendableMessageEvent object has a null source. r=asuth

Fix a potential crash. Aurora54+ & Beta53+.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/45aedfc50a44

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/03bd925be740

Comment 14

[Cornel Ionce [:cornel_ionce], Desktop Release QA]

Setting qe-verify- based on comment 10.