New issue
Advanced search Search tips

Issue 594032 link

Starred by 0 users

Issue metadata

Status: Archived
Owner: ----
Closed: Jun 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Improve layout tests for Service Worker

Project Member Reported by nhiroki@chromium.org, Mar 11 2016

Issue description

We could improve/stabilize our layout tests more:

- We should unregister SW and remove frames with completion callback mechanism (ie. add_completion_callback()) to make sure to clean up resources even if the tests fail. Blink's test runner ("run-webkit-tests") creates a temporary user profile every time, so this is not a problem on the try servers. However, this is cumbersome when we manually run tests again and again for debugging, checking interoperability, etc.

- We could remove some pointless/duplicate tests.

- We could replace old utilities with new ones (eg. async_test() -> promise_test())
 
Re: the first item, following tests would be affected by the frame issue:

- controller-on-load.html
- postmessage-msgport-to-client.html
- unregister-controller.html
- unregister-then-register.html

(These were found by https://docs.google.com/spreadsheets/d/1lcNWsryL9G6qE1ge1n2_rZ7NF1VSTPnRNx7cCXswqm0/edit?usp=sharing)
Owner: shimazu@chromium.org
Status: Started (was: Assigned)
shimazu@ is working on the first item.

Comment 3 by shimazu@google.com, Apr 6 2016

I started this patch; according to an offline discussion with nhiroki@, this problem might be caused by a difference behavior of "unload" of iframe controlling SW between firefox and chromium.

The issue can be observed by following this instructions:
 - register a SW script
 - create an iframe served by the SW script
 - unregister the SW
 - reload the page
 - re-register and wait for the new SW script

In this situation, 
wait_for_state(t, registration.installing, 'activated')
will wait though the registration.installing, active and waiting are null on firefox. 
Probably firefox holds the older SW despite of reloading. 


Is it better to add a test to monitor the difference, or need any other operations for this issue?
> wait_for_state(t, registration.installing, 'activated') will wait though the registration.installing, active and waiting are null on firefox. Probably firefox holds the older SW despite of reloading. 

You mean registration.active is set but installing and waiting are null, right?

> Is it better to add a test to monitor the difference, or need any other operations for this issue?

Adding a test sounds good. It'd be more nice if you could file an issue in their bug tracker.

Comment 5 by shimazu@google.com, Apr 7 2016

> You mean registration.active is set but installing and waiting are null, right?

Yes, that's right.

> Adding a test sounds good. It'd be more nice if you could file an issue in their bug tracker.
Could you add a ticket by a one-line comment? I can't do that because of my account issue now... x(
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/025c7a5cefe328486bcd4309c2b945987a09374c

commit 025c7a5cefe328486bcd4309c2b945987a09374c
Author: shimazu <shimazu@google.com>
Date: Thu Apr 07 09:41:40 2016

ServiceWorker: Remove iframes explicitly after a test is finished

On Firefox, reloading a test page which has an iframe controlling
unregistered SW doesn't release the worker though the iframe is already
released, and this results failure of the subsequent test.
Removing iframes after the test is finished would be a solution for this problem.

BUG= 594032 
TEST=./third_party/WebKit/Tools/Scripts/run-webkit-tests -t Default http/tests/serviceworker

Review URL: https://codereview.chromium.org/1859343002

Cr-Commit-Position: refs/heads/master@{#385699}

[modify] https://crrev.com/025c7a5cefe328486bcd4309c2b945987a09374c/third_party/WebKit/LayoutTests/http/tests/serviceworker/controller-on-load.html
[modify] https://crrev.com/025c7a5cefe328486bcd4309c2b945987a09374c/third_party/WebKit/LayoutTests/http/tests/serviceworker/fetch-mixed-content-to-outscope-expected.txt
[modify] https://crrev.com/025c7a5cefe328486bcd4309c2b945987a09374c/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/navigation-redirect-to-http-iframe.html
[modify] https://crrev.com/025c7a5cefe328486bcd4309c2b945987a09374c/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js
[modify] https://crrev.com/025c7a5cefe328486bcd4309c2b945987a09374c/third_party/WebKit/LayoutTests/http/tests/serviceworker/sync-xhr-doesnt-deadlock-expected.txt

Project Member

Comment 7 by bugdroid1@chromium.org, Apr 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a23a81226c806c8a34291b177ee058dd45b6527

commit 6a23a81226c806c8a34291b177ee058dd45b6527
Author: shimazu <shimazu@google.com>
Date: Tue Apr 12 06:25:20 2016

ServiceWorker: Add a test to check if worker is updated after reloading

After unregistration of a service worker, reloading an iframe provided
by the SW should release that worker. This patch is a test for checking
this behavior.

And, I expected the result of this new test is different between
chromium and firefox based on  crbug.com/594032 , but both two
environments passed the test. I'm not sure the difference between reload
button and frame.contentWindow.reload().

BUG= 594032 

Review URL: https://codereview.chromium.org/1865103003

Cr-Commit-Position: refs/heads/master@{#386599}

[modify] https://crrev.com/6a23a81226c806c8a34291b177ee058dd45b6527/third_party/WebKit/LayoutTests/http/tests/serviceworker/unregister-then-register.html

Done for the reloading issue for this bug.
For the reloading behavior, I created another bug: http://crbug.com/603793


Then, the remaining:

- We could remove some pointless/duplicate tests.
- We could replace old utilities with new ones (eg. async_test() -> promise_test())

Comment 9 by falken@chromium.org, Apr 18 2016

I'm not sure we should do major refactorings of our tests until after we've synced up with web-platform-tests.
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ece7c41ea8ca49b959f12576284e4966a25d3606

commit ece7c41ea8ca49b959f12576284e4966a25d3606
Author: shimazu <shimazu@google.com>
Date: Mon Apr 18 09:26:52 2016

ServiceWorker: Check the equality of JS-level SW/SWR objects

This test checks if the series of |registration.installing|,
|registration.waiting| and |registration.active| are the same when
installing a new service worker. I update the equality check to compare
the object itself instead of scriptURL because the spec explicitly shows
object level equality now.

Additionally, this test fails on Firefox 45.0 due to mismatching of
JS-level equality of each service worker object.

BUG= 594032 

Review URL: https://codereview.chromium.org/1872243002

Cr-Commit-Position: refs/heads/master@{#387875}

[modify] https://crrev.com/ece7c41ea8ca49b959f12576284e4966a25d3606/third_party/WebKit/LayoutTests/http/tests/serviceworker/registration-service-worker-attributes.html
[modify] https://crrev.com/ece7c41ea8ca49b959f12576284e4966a25d3606/third_party/WebKit/LayoutTests/http/tests/serviceworker/synced-state.html

Owner: ----
Status: Available (was: Started)
Labels: -Pri-2 Pri-3
Status: Archived (was: Available)

Sign in to add a comment