Closed
Bug 1193133
Opened 9 years ago
Closed 9 years ago
ServiceWorker::PostMessage should fail when calling from a dom object that doesn't have a global
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: catalinb, Assigned: catalinb)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Right now we crash - which is not desireable.
See: https://github.com/slightlyoff/ServiceWorker/issues/722
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8646132 -
Flags: review?(bkelly)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8646133 -
Flags: review?(bkelly)
Assignee | ||
Comment 3•9 years ago
|
||
Comment 4•9 years ago
|
||
Comment on attachment 8646133 [details] [diff] [review]
Throw when calling postMessage from a Service Worker dom object with no global.
Review of attachment 8646133 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comments addressed. Thanks.
::: dom/workers/ServiceWorker.cpp
@@ +108,2 @@
> nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
> + MOZ_ASSERT(doc);
What guarantees do we have that this is not nullptr? I'm fairly certain GetExtantDoc can return nullptr under certain circumstances.
Is there any reason not to just return an error here instead? (moving the worker private bits down further of course)
Attachment #8646133 -
Flags: review?(bkelly) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8646132 [details] [diff] [review]
Drop the document and window references from ServiceWorker.
Review of attachment 8646132 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with comments addressed. Basically, I think we need some more null doc checks. Thanks.
::: dom/workers/ServiceWorker.cpp
@@ +99,5 @@
> }
>
> + nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(GetParentObject());
> + nsCOMPtr<nsIDocument> doc = window->GetExtantDoc();
> + nsAutoPtr<ServiceWorkerClientInfo> clientInfo(new ServiceWorkerClientInfo(doc));
Again, it seems like if the window goes away that we can get either a nullptr window or doc here, no? It seems like we should check and throw.
::: dom/workers/ServiceWorkerClient.h
@@ -13,5 @@
> #include "mozilla/ErrorResult.h"
> #include "mozilla/dom/BindingDeclarations.h"
> #include "mozilla/dom/ClientBinding.h"
>
> -class nsIDocument;
Seems we should still need nsIDocument here, right? I guess it doesn't matter with unified builds, but would be nice to try to maintain non-unified compilation if we can.
::: dom/workers/ServiceWorkerWindowClient.cpp
@@ +93,5 @@
> + nsContentUtils::DispatchChromeEvent(window->GetExtantDoc(),
> + window->GetOuterWindow(),
> + NS_LITERAL_STRING("DOMServiceWorkerFocusClient"),
> + true, true);
> + clientInfo.reset(new ServiceWorkerClientInfo(window->GetDocument()));
It seems weird to me we call GetDocument(), which might create a document, after calling GetExtandDoc() above, which does not create a document. Should we just call GetDocument() once and use it in both call sites?
Also it seems if the docshell is gone, then GetDocument() can return nullptr as well. So we should probably check for null document here.
Attachment #8646132 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8646132 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8646133 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
I've made the requested null checks for the doc. However, *as far as i can tell*, having a valid inner window should always guarantee a valid document.
Comment 10•9 years ago
|
||
Backed out for frequent wpt failures as documented in bug 1194536.
Comment 11•9 years ago
|
||
Assignee | ||
Comment 12•9 years ago
|
||
Assignee | ||
Comment 13•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b07f5ae8bb03
Also disabling /testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-event.https.html which was already marked as failing.
Assignee | ||
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
You can put r=bkelly on the patch disabling the tests if you want.
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2d380f4a2f39
https://hg.mozilla.org/mozilla-central/rev/f7aec85fbf71
https://hg.mozilla.org/mozilla-central/rev/9401a3b12b4f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•