Closed
Bug 1347739
Opened 6 years ago
Closed 6 years ago
ExtendableMessageEvent `source` accessor crashes tab
Categories
(Core :: DOM: Service Workers, defect)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: jugglinmike, Assigned: bkelly)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
1.13 KB,
patch
|
asuth
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
Mike, do you have a test for this already written?
Flags: needinfo?(mike)
Assignee | ||
Updated•6 years ago
|
Blocks: ServiceWorkers-stability
Assignee | ||
Comment 3•6 years ago
|
||
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.
Attachment #8848073 -
Flags: review?(bugmail)
Assignee | ||
Comment 4•6 years ago
|
||
I also checked MessageEvent. It does not have this problem: https://dxr.mozilla.org/mozilla-central/source/dom/events/MessageEvent.cpp#90
Updated•6 years ago
|
Attachment #8848073 -
Flags: review?(bugmail) → review+
Reporter | ||
Comment 5•6 years ago
|
||
(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/
Flags: needinfo?(mike)
Assignee | ||
Comment 6•6 years ago
|
||
Ok. I'm just going to land this then. We'll get the test when it merges around. Thanks!
Pushed by bkelly@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/77b0e80d2bd3 Don't crash if an ExtendableMessageEvent object has a null source. r=asuth
Assignee | ||
Comment 8•6 years ago
|
||
We might as well uplift this to beta/aurora since its such an easy fix.
status-firefox52:
--- → wontfix
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox55:
--- → affected
status-firefox-esr45:
--- → wontfix
status-firefox-esr52:
--- → wontfix
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/77b0e80d2bd3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 10•6 years ago
|
||
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
Attachment #8848073 -
Flags: approval-mozilla-beta?
Attachment #8848073 -
Flags: approval-mozilla-aurora?
Comment 11•6 years ago
|
||
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+.
Attachment #8848073 -
Flags: approval-mozilla-beta?
Attachment #8848073 -
Flags: approval-mozilla-beta+
Attachment #8848073 -
Flags: approval-mozilla-aurora?
Attachment #8848073 -
Flags: approval-mozilla-aurora+
Comment 12•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/45aedfc50a44
Comment 13•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/03bd925be740
You need to log in
before you can comment on or make changes to this bug.
Description
•