1193133 - ServiceWorker::PostMessage should fail when calling from a dom object that doesn't have a global

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

Right now we crash - which is not desireable.
See: https://github.com/slightlyoff/ServiceWorker/issues/722

Comment 1

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

Attachment: Drop the document and window references from ServiceWorker.

Comment 2

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

Attachment: Throw when calling postMessage from a Service Worker dom object with no global.

Comment 3

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

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

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

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)

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 6

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

Attachment: Drop the document and window references from ServiceWorker.

Comment 7

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

Attachment: Throw when calling postMessage from a Service Worker dom object with no global.

Comment 8

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

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 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2469ea7f01
https://hg.mozilla.org/integration/mozilla-inbound/rev/2381f29f63c2

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for frequent wpt failures as documented in bug 1194536.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backout: https://hg.mozilla.org/mozilla-central/rev/1b2402247429

Comment 12

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

Attachment: Disable broken service worker wpt tests. a=testonly

Comment 13

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

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.

Comment 14

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

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

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

You can put r=bkelly on the patch disabling the tests if you want.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d380f4a2f39
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7aec85fbf71
https://hg.mozilla.org/integration/mozilla-inbound/rev/9401a3b12b4f

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/2d380f4a2f39
https://hg.mozilla.org/mozilla-central/rev/f7aec85fbf71
https://hg.mozilla.org/mozilla-central/rev/9401a3b12b4f