Add null checks in navigator.serviceWorker access to fix possible crash

Add null checks in navigator.serviceWorker access to fix possible crash

This fixes two possible crash:
1. document->frame() could be null when ServiceWorkerContainerClient::from is called
2. NavigatorServiceWorker.m_serviceWorker could be null in ServiceWorkerContainer::detachClient

BUG=355998, 356017
TEST=fast/serviceworker/access-container-with-invalid-context.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170501

[kinuko, 2014-03-31 03:56:31]

Could dominicc@ or someone review this?

[dominicc (has gone to gerrit), 2014-03-31 04:52:01]

On 2014/03/31 03:56:31, kinuko wrote:
> Could dominicc@ or someone review this?

Comments inline.

Handling context death... always painful.

[dominicc (has gone to gerrit), 2014-03-31 04:52:09]

https://codereview.chromium.org/217023003/diff/40001/LayoutTests/fast/service...
File LayoutTests/fast/serviceworker/access-container-with-invalid-context.html
(right):

https://codereview.chromium.org/217023003/diff/40001/LayoutTests/fast/service...
LayoutTests/fast/serviceworker/access-container-with-invalid-context.html:6:
testRunner.dumpAsText();
I could be wrong, but isn't dumpAsText the default?

https://codereview.chromium.org/217023003/diff/40001/LayoutTests/fast/service...
LayoutTests/fast/serviceworker/access-container-with-invalid-context.html:7:
testRunner.setCanOpenWindows();
Do you need separate windows? Why not just use an iframe?

https://codereview.chromium.org/217023003/diff/40001/LayoutTests/fast/service...
LayoutTests/fast/serviceworker/access-container-with-invalid-context.html:29:
setTimeout(callback, 1);
Why do it 100 times? Do you just need a strategic gc() in here or something?

https://codereview.chromium.org/217023003/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/217023003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:97:
resolver->reject(DOMError::create(NotSupportedError, "No provider is
available"));
It would be great to see a W3C-style test for this so that we can check that the
assert(s) in the test line up with language in the spec.

https://codereview.chromium.org/217023003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:113: : m_provider(0)
Why use nullptr sometimes and 0 other times?

https://codereview.chromium.org/217023003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:117: if
(ServiceWorkerContainerClient::from(executionContext)) {
More idiomatic Blink would be something like:

if (Foo* bar = ServiceWorkerContainerClient::from(executionContext)) {
  m_provider = bar->provider();
  ...

etc.

[kinuko, 2014-03-31 12:33:04]

Addressed some but not all. (I'm not unable to create a reliable test for this
one, experts' comments/suggestions are welcome)

https://codereview.chromium.org/217023003/diff/40001/LayoutTests/fast/service...
File LayoutTests/fast/serviceworker/access-container-with-invalid-context.html
(right):

https://codereview.chromium.org/217023003/diff/40001/LayoutTests/fast/service...
LayoutTests/fast/serviceworker/access-container-with-invalid-context.html:6:
testRunner.dumpAsText();
On 2014/03/31 04:52:09, dominicc wrote:
> I could be wrong, but isn't dumpAsText the default?

I removed this as I've converted it to use w3c test harness.

https://codereview.chromium.org/217023003/diff/40001/LayoutTests/fast/service...
LayoutTests/fast/serviceworker/access-container-with-invalid-context.html:7:
testRunner.setCanOpenWindows();
On 2014/03/31 04:52:09, dominicc wrote:
> Do you need separate windows? Why not just use an iframe?

Hmm, on my local env I keep failing to repro the crash with iframe. (Is there a
known test pattern I could try?)

https://codereview.chromium.org/217023003/diff/40001/LayoutTests/fast/service...
LayoutTests/fast/serviceworker/access-container-with-invalid-context.html:29:
setTimeout(callback, 1);
On 2014/03/31 04:52:09, dominicc wrote:
> Why do it 100 times? Do you just need a strategic gc() in here or something?

Either with gc() or not I can't seem to reliably repro this (::detachClient one
is reproducible but the other one is not).

I removed the ad-hoc repetition from this test code anyway.

https://codereview.chromium.org/217023003/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/217023003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:97:
resolver->reject(DOMError::create(NotSupportedError, "No provider is
available"));
On 2014/03/31 04:52:09, dominicc wrote:
> It would be great to see a W3C-style test for this so that we can check that
the
> assert(s) in the test line up with language in the spec.

Yes, but if we can have a test code that can repro this situation 100%.

https://codereview.chromium.org/217023003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:113: : m_provider(0)
On 2014/03/31 04:52:09, dominicc wrote:
> Why use nullptr sometimes and 0 other times?

Some compilers don't accept assigning nullptr to a raw ptr. (While for RefPtr* /
OwnPtr* we always use nullptr now)

https://codereview.chromium.org/217023003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:117: if
(ServiceWorkerContainerClient::from(executionContext)) {
On 2014/03/31 04:52:09, dominicc wrote:
> More idiomatic Blink would be something like:
> 
> if (Foo* bar = ServiceWorkerContainerClient::from(executionContext)) {
>   m_provider = bar->provider();
>   ...
> 
> etc.

Done.

[dominicc (has gone to gerrit), 2014-03-31 23:19:10]

LGTM

I have more thoughts about tests but let's discuss IRL.

[dominicc (has gone to gerrit), 2014-03-31 23:19:14]

The CQ bit was checked by dominicc@chromium.org

[commit-bot: I haz the power, 2014-03-31 23:19:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/217023003/150001

[commit-bot: I haz the power, 2014-04-01 01:30:18]

Change committed as 170501

[kinuko, 2014-04-01 03:26:51]

Btw... I converted the test (the crash one) into w3c test mainly for a try out,
but it probably should NOT be in w3c style?  I can rewrite it back to a regular
test then (I realized that it might be just adding noise when filtering feature
tests later)

[dominicc (has gone to gerrit), 2014-04-01 05:07:43]

On 2014/04/01 03:26:51, kinuko wrote:
> Btw... I converted the test (the crash one) into w3c test mainly for a try
out,
> but it probably should NOT be in w3c style?  I can rewrite it back to a
regular
> test then (I realized that it might be just adding noise when filtering
feature
> tests later)

I tried reverting the C++ parts of this patch and the test still passed. I'm
going to patch an earlier version of the test and hack on the test.

[kinuko, 2014-04-01 05:09:13]

On 2014/04/01 05:07:43, dominicc wrote:
> On 2014/04/01 03:26:51, kinuko wrote:
> > Btw... I converted the test (the crash one) into w3c test mainly for a try
> out,
> > but it probably should NOT be in w3c style?  I can rewrite it back to a
> regular
> > test then (I realized that it might be just adding noise when filtering
> feature
> > tests later)
> 
> I tried reverting the C++ parts of this patch and the test still passed. I'm
> going to patch an earlier version of the test and hack on the test.

The test only crashes with ASAN build.

[dominicc (has gone to gerrit), 2014-04-01 11:45:53]

On 2014/04/01 05:09:13, kinuko wrote:
> The test only crashes with ASAN build.

Ah, of course! Sorry for the spam.