ServiceWorker: Fix registration-iframe.https.html

ServiceWorker: Fix registration-iframe.https.html

registration-iframe.https.html became flaky after https://codereview.chromium.org/2691903005.

This CL fixes the test by correcting the scope URL given to unregister
function in the first async_test and using different scope urls for
different tests to avoid conflicts in case there is cross-talk between
tests.

BUG=697087
Review-Url: https://codereview.chromium.org/2732513003
Cr-Commit-Position: refs/heads/master@{#455046}
Committed: https://chromium.googlesource.com/chromium/src/+/0d01f9fe710dfbe680f317621a617e383422e14d

[jungkees, 2017-03-03 06:17:43]

Description was changed from

==========
ServiceWorker: Fix registration-iframe.https.html

registration-iframe.https.html became flaky after
https://codereview.chromium.org/2691903005.

This CL fixes the test by correcting the scope URL given to unregister
function in the first async_test. Before this change, a wrong URL was
passed so the registration was not cleared after the test.

BUG=697087
==========

to

==========
ServiceWorker: Fix registration-iframe.https.html

registration-iframe.https.html became flaky after
https://codereview.chromium.org/2691903005.

This CL fixes the test by correcting the scope URL given to unregister
function in the first async_test. Before this change, a wrong URL was
passed so the registration was not cleared after the test.

BUG=697087
==========

[jungkees, 2017-03-03 06:17:44]

jungkee.song@samsung.com changed reviewers:
+ falken@chromium.org, yutak@chromium.org

[jungkees, 2017-03-03 06:27:09]

falken@, I couldn't reproduce the issue in my local machines (Linux and Mac) but
found the first async_test doesn't correctly unregister the registration at the
end of the test as a wrong URL is given to the unregister function. Also I
adjusted the scope URLs given to unregister functions in other async_tests too
such that they try to unregister the registration with the scope that the
iframes' register attempt is supposed to run on.

I'm planning to change the TestExpectations file back when I see the result gets
stable. Is that OK?

PTAL.

[jungkees, 2017-03-03 06:27:40]

The CQ bit was checked by jungkee.song@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 06:27:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 07:26:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 07:26:53]

Dry run: This issue passed the CQ dry run.

[falken, 2017-03-07 05:00:19]

I think we can make the tests simpler and less error prone.

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html
(right):

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:13:
const iframe_scope = 'registration-for-iframe-from-parent-frame';
let's make it so the three tests don't all use the string
"registration-for-iframe-from-parent-frame" in case there is cross-talk between
the tests.

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:40:
return service_worker_unregister_and_done(t, scope);
Instead of unregister_and_done, how about we just do

...
    return registration.unregister();
  }
.then {
  t.done();
}

?

To avoid the error where the wrong scope is used.

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:73:
return service_worker_unregister_and_done(t, scope);
let's just call t.done() since there is no registration to remove.

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:106:
return service_worker_unregister_and_done(t, scope);
let's just call t.done()

[jungkees, 2017-03-07 07:01:28]

Description was changed from

==========
ServiceWorker: Fix registration-iframe.https.html

registration-iframe.https.html became flaky after
https://codereview.chromium.org/2691903005.

This CL fixes the test by correcting the scope URL given to unregister
function in the first async_test. Before this change, a wrong URL was
passed so the registration was not cleared after the test.

BUG=697087
==========

to

==========
ServiceWorker: Fix registration-iframe.https.html

registration-iframe.https.html became flaky after
https://codereview.chromium.org/2691903005.

This CL fixes the test by correcting the scope URL given to unregister
function in the first async_test and using different scope urls for
different tests to avoid conflicts in case there is cross-talk between
tests.

BUG=697087
==========

[jungkees, 2017-03-07 07:02:03]

I uploaded a new snapshot having addressed the comments. PTAL.

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html
(right):

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:13:
const iframe_scope = 'registration-for-iframe-from-parent-frame';
On 2017/03/07 05:00:19, falken wrote:
> let's make it so the three tests don't all use the string
> "registration-for-iframe-from-parent-frame" in case there is cross-talk
between
> the tests.

Understood your point. Addressed.

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:40:
return service_worker_unregister_and_done(t, scope);
On 2017/03/07 05:00:19, falken wrote:
> Instead of unregister_and_done, how about we just do
> 
> ...
>     return registration.unregister();
>   }
> .then {
>   t.done();
> }
> 
> ?
> 
> To avoid the error where the wrong scope is used.

While testing this proposed change, I noticed that registration.unregister()
can't be used across the global objects as the method is using the context
object's relevant settings to create the unregister job instead of using the
iframe's settings. In the implementation point of view, the method fails as
there's no assoicated provider.

For this test, I did:

...
    return service_worker_unregister(t, scope);
  })
.then(function() {
    t.done();
  })

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:73:
return service_worker_unregister_and_done(t, scope);
On 2017/03/07 05:00:19, falken wrote:
> let's just call t.done() since there is no registration to remove.

Done.

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:106:
return service_worker_unregister_and_done(t, scope);
On 2017/03/07 05:00:19, falken wrote:
> let's just call t.done()

Done.

[falken, 2017-03-07 07:11:48]

Thanks this lg with a nit.

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html
(right):

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:40:
return service_worker_unregister_and_done(t, scope);
On 2017/03/07 07:02:03, jungkees wrote:
> On 2017/03/07 05:00:19, falken wrote:
> > Instead of unregister_and_done, how about we just do
> > 
> > ...
> >     return registration.unregister();
> >   }
> > .then {
> >   t.done();
> > }
> > 
> > ?
> > 
> > To avoid the error where the wrong scope is used.
> 
> While testing this proposed change, I noticed that registration.unregister()
> can't be used across the global objects as the method is using the context
> object's relevant settings to create the unregister job instead of using the
> iframe's settings. In the implementation point of view, the method fails as
> there's no assoicated provider.
> 
> For this test, I did:
> 
> ...
>     return service_worker_unregister(t, scope);
>   })
> .then(function() {
>     t.done();
>   })

Interesting. This could likely be resolved by:
  registration.unregister();
.then(() =>
  frame.remove();
  t.done();

But maybe that's more complicated than just unregister_and_done.

Let's just use service_worker_unregister_and_done(t, scope) then as you
originally had here.

https://codereview.chromium.org/2732513003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html
(right):

https://codereview.chromium.org/2732513003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:11:
async_test(function(t) {
We could optionally change all these async_test  to promise_test. Then we don't
need to call t.done(). And also the tests run sequentially so there is no
cross-talk between tests. But it's optional for this patch.

[jungkees, 2017-03-07 07:26:27]

Uploaded the fix. PTAL.

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html
(right):

https://codereview.chromium.org/2732513003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/registration-iframe.https.html:40:
return service_worker_unregister_and_done(t, scope);
On 2017/03/07 07:11:47, falken wrote:
> On 2017/03/07 07:02:03, jungkees wrote:
> > On 2017/03/07 05:00:19, falken wrote:
> > > Instead of unregister_and_done, how about we just do
> > > 
> > > ...
> > >     return registration.unregister();
> > >   }
> > > .then {
> > >   t.done();
> > > }
> > > 
> > > ?
> > > 
> > > To avoid the error where the wrong scope is used.
> > 
> > While testing this proposed change, I noticed that registration.unregister()
> > can't be used across the global objects as the method is using the context
> > object's relevant settings to create the unregister job instead of using the
> > iframe's settings. In the implementation point of view, the method fails as
> > there's no assoicated provider.
> > 
> > For this test, I did:
> > 
> > ...
> >     return service_worker_unregister(t, scope);
> >   })
> > .then(function() {
> >     t.done();
> >   })
> 
> Interesting. This could likely be resolved by:
>   registration.unregister();
> .then(() =>
>   frame.remove();
>   t.done();
> 
> But maybe that's more complicated than just unregister_and_done.
> 
> Let's just use service_worker_unregister_and_done(t, scope) then as you
> originally had here.
> 

Embarrassing ;) I missed that frame remove. The context's relevant settings
should be the iframe's settings indeed. Just done as in your original comment.

[falken, 2017-03-07 07:29:57]

lgtm

[jungkees, 2017-03-07 07:30:16]

The CQ bit was checked by jungkee.song@samsung.com

[commit-bot: I haz the power, 2017-03-07 07:31:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 09:28:48]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488871816534680,
"parent_rev": "504550f038ae920fbc2bee9797f298189262e4fc", "commit_rev":
"0d01f9fe710dfbe680f317621a617e383422e14d"}

[commit-bot: I haz the power, 2017-03-07 09:29:31]

Description was changed from

==========
ServiceWorker: Fix registration-iframe.https.html

registration-iframe.https.html became flaky after
https://codereview.chromium.org/2691903005.

This CL fixes the test by correcting the scope URL given to unregister
function in the first async_test and using different scope urls for
different tests to avoid conflicts in case there is cross-talk between
tests.

BUG=697087
==========

to

==========
ServiceWorker: Fix registration-iframe.https.html

registration-iframe.https.html became flaky after
https://codereview.chromium.org/2691903005.

This CL fixes the test by correcting the scope URL given to unregister
function in the first async_test and using different scope urls for
different tests to avoid conflicts in case there is cross-talk between
tests.

BUG=697087

Review-Url: https://codereview.chromium.org/2732513003
Cr-Commit-Position: refs/heads/master@{#455046}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d01f9fe710dfbe680f317621a61...
==========

[commit-bot: I haz the power, 2017-03-07 09:29:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0d01f9fe710dfbe680f317621a61...