ServiceWorker: Remove iframes explicitly after a test is finished

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

Committed: https://crrev.com/025c7a5cefe328486bcd4309c2b945987a09374c
Cr-Commit-Position: refs/heads/master@{#385699}

[shimazu (google), 2016-04-06 06:28:37]

The CQ bit was checked by shimazu@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-06 06:29:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1859343002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1859343002/1

[shimazu (google), 2016-04-06 06:33:34]

shimazu@google.com changed reviewers:
+ nhiroki@chromium.org

[shimazu (google), 2016-04-06 06:33:35]

PTAL :)

# Sadly I'm locked out of my chromium account because of the 2-factor
authentication without my previous broken smartphone, so I issued this patch via
my google account...

[commit-bot: I haz the power, 2016-04-06 07:35:21]

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

[commit-bot: I haz the power, 2016-04-06 07:35:22]

Dry run: This issue passed the CQ dry run.

[nhiroki, 2016-04-06 07:46:35]

Looks good. Added minor comments.

https://codereview.chromium.org/1859343002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js
(right):

https://codereview.chromium.org/1859343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js:47:
// iframe when it finishes loading. By default, the caller is responsible for
"By default" sounds a bit unclear. Can you elaborate the meaning of
|reclaim_after_completion|? For example,

// iframe when it finishes loading. If |reclaim_after_completion| is true,
registers a completion callback to remove the frame. Otherwise, the caller is
responsible for removing the iframe later.

https://codereview.chromium.org/1859343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js:55:
if (reclaim_after_completion)
if (reclaim_after_compleation === true)

... could be more stable in JS.

[falken, 2016-04-06 07:49:05]

falken@chromium.org changed reviewers:
+ falken@chromium.org

[falken, 2016-04-06 07:49:05]

https://codereview.chromium.org/1859343002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js
(right):

https://codereview.chromium.org/1859343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js:47:
// iframe when it finishes loading. By default, the caller is responsible for
On 2016/04/06 07:46:35, nhiroki wrote:
> "By default" sounds a bit unclear. Can you elaborate the meaning of
> |reclaim_after_completion|? For example,
> 
> // iframe when it finishes loading. If |reclaim_after_completion| is true,
> registers a completion callback to remove the frame. Otherwise, the caller is
> responsible for removing the iframe later.

Drive-by comment: How about making reclaim_after_completion the default
behavior? Also I'd slightly prefer passing some const value instead of
true/false for readability.

[shimazu (google), 2016-04-07 04:17:59]

Thanks, and I changed the default behavior of with_iframe.
This won't cause problems in any case but 'with_iframe' on iframe (this doesn't
contain the test-harness).
This is why reclamation is disabled in navigation-redirect-to-http-iframe.html.

PTAL again.

https://codereview.chromium.org/1859343002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js
(right):

https://codereview.chromium.org/1859343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js:47:
// iframe when it finishes loading. By default, the caller is responsible for
On 2016/04/06 07:49:05, falken wrote:
> On 2016/04/06 07:46:35, nhiroki wrote:
> > "By default" sounds a bit unclear. Can you elaborate the meaning of
> > |reclaim_after_completion|? For example,
> > 
> > // iframe when it finishes loading. If |reclaim_after_completion| is true,
> > registers a completion callback to remove the frame. Otherwise, the caller
is
> > responsible for removing the iframe later.
> 
> Drive-by comment: How about making reclaim_after_completion the default
> behavior? Also I'd slightly prefer passing some const value instead of
> true/false for readability.

Done.

https://codereview.chromium.org/1859343002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js:55:
if (reclaim_after_completion)
On 2016/04/06 07:46:35, nhiroki wrote:
> if (reclaim_after_compleation === true)
> 
> ... could be more stable in JS.

Done.

[nhiroki, 2016-04-07 05:37:18]

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/controller-on-load.html
(right):

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/controller-on-load.html:17:
return with_iframe(scope, true);
|IFRAME_RECLAMATION.TRUE| is the default behavior, so we don't have to pass
|true| here and elsewhere.

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js
(right):

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js:50:
var IFRAME_RECLAMATION = Object.freeze({TRUE: true, FALSE: false});
How about using 'const' keyword instead?

  const DONT_RECLAIM_IFRAME = true;

And, maybe we can reduce the condition like this:

  function with_iframe(url, dont_reclaim_iframe) {
      // ...
      if (DONT_RECLAIM_IFRAME === dont_reclaim_iframe)
        return;
      add_completion_callback(function() { frame.remove(); });
  }

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/unregister-then-register.html
(right):

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/unregister-then-register.html:115:
iframe.remove();
We no longer have to call iframe.remove() here and elsewhere. Can you remove
them?

[falken, 2016-04-07 05:47:26]

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js
(right):

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js:50:
var IFRAME_RECLAMATION = Object.freeze({TRUE: true, FALSE: false});
On 2016/04/07 05:37:18, nhiroki wrote:
> How about using 'const' keyword instead?
> 
>   const DONT_RECLAIM_IFRAME = true;
> 
> And, maybe we can reduce the condition like this:
> 
>   function with_iframe(url, dont_reclaim_iframe) {
>       // ...
>       if (DONT_RECLAIM_IFRAME === dont_reclaim_iframe)
>         return;
>       add_completion_callback(function() { frame.remove(); });
>   }

Sorry for more drive-bys, just thought of this. Passing an optional options
object like:
{auto_remove: false}
seems a bit more common in the JS world?

[shimazu (google), 2016-04-07 06:53:55]

Incorporated with the reviews.
PTAL again.

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/controller-on-load.html
(right):

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/controller-on-load.html:17:
return with_iframe(scope, true);
On 2016/04/07 05:37:18, nhiroki wrote:
> |IFRAME_RECLAMATION.TRUE| is the default behavior, so we don't have to pass
> |true| here and elsewhere.

Done.

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js
(right):

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/test-helpers.js:50:
var IFRAME_RECLAMATION = Object.freeze({TRUE: true, FALSE: false});
On 2016/04/07 05:47:26, falken wrote:
> On 2016/04/07 05:37:18, nhiroki wrote:
> > How about using 'const' keyword instead?
> > 
> >   const DONT_RECLAIM_IFRAME = true;
> > 
> > And, maybe we can reduce the condition like this:
> > 
> >   function with_iframe(url, dont_reclaim_iframe) {
> >       // ...
> >       if (DONT_RECLAIM_IFRAME === dont_reclaim_iframe)
> >         return;
> >       add_completion_callback(function() { frame.remove(); });
> >   }
> 
> Sorry for more drive-bys, just thought of this. Passing an optional options
> object like:
> {auto_remove: false}
> seems a bit more common in the JS world?

I think so, and fixed it.

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/unregister-then-register.html
(right):

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/unregister-then-register.html:115:
iframe.remove();
On 2016/04/07 05:37:18, nhiroki wrote:
> We no longer have to call iframe.remove() here and elsewhere. Can you remove
> them?

In my understanding, this test requires the iframe.remove() to ensure
resurrection of iframe (this removal existed previously).

[nhiroki, 2016-04-07 07:21:17]

lgtm

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/serviceworker/unregister-then-register.html
(right):

https://codereview.chromium.org/1859343002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/serviceworker/unregister-then-register.html:115:
iframe.remove();
On 2016/04/07 06:53:55, makoto (google) wrote:
> On 2016/04/07 05:37:18, nhiroki wrote:
> > We no longer have to call iframe.remove() here and elsewhere. Can you remove
> > them?
> 
> In my understanding, this test requires the iframe.remove() to ensure
> resurrection of iframe (this removal existed previously).

You're right and sorry! I randomly picked up a callsite of remove() to ask you
to remove real no-op remove() calls. Can you make a separate CL to remove them
later?

[falken, 2016-04-07 08:38:43]

Great patch! Many of our tests remove the iframe explicitly or add their own
completion callback. Can you remove that code? (It could be a different patch.)

[shimazu (google), 2016-04-07 08:46:06]

The CQ bit was checked by shimazu@google.com

[commit-bot: I haz the power, 2016-04-07 08:46:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1859343002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1859343002/60001

[commit-bot: I haz the power, 2016-04-07 09:41:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-07 09:43:58]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/025c7a5cefe328486bcd4309c2b945987a09374c
Cr-Commit-Position: refs/heads/master@{#385699}
==========

[commit-bot: I haz the power, 2016-04-07 09:44:00]

Patchset 4 (id:??) landed as
https://crrev.com/025c7a5cefe328486bcd4309c2b945987a09374c
Cr-Commit-Position: refs/heads/master@{#385699}