Worklet: Avoid import() on a detached frame

Worklet: Avoid import() on a detached frame

Without this fix, import() on a detached frame eventually crashes because of
a null execution context.

BUG=689265
Review-Url: https://codereview.chromium.org/2697243003
Cr-Commit-Position: refs/heads/master@{#450972}
Committed: https://chromium.googlesource.com/chromium/src/+/5989961e2951701ff4a2d74bde290a0d26b45370

[nhiroki, 2017-02-16 07:18:38]

The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run

[nhiroki, 2017-02-16 07:18:49]

Patchset #1 (id:1) has been deleted

[nhiroki, 2017-02-16 07:19:01]

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

[nhiroki, 2017-02-16 07:19:01]

PTAL

[commit-bot: I haz the power, 2017-02-16 07:19:36]

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

[falken, 2017-02-16 07:25:03]

Nice bug. Does it make sense to commit the test to external/wpt instead?

https://codereview.chromium.org/2697243003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/worklet/import-on-detached-iframe.html
(right):

https://codereview.chromium.org/2697243003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/worklet/import-on-detached-iframe.html:12:
var frame = document.createElement('iframe');
nit: let or const

https://codereview.chromium.org/2697243003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/worklet/import-on-detached-iframe.html:16:
add_completion_callback(() => frame.remove());
nit: add_result_callback may be slightly cleaner in case more tests are added to
this file. If I understand correctly add_result_callback is invoked when this
promise_test finishes, and add_completion_callback is invoked when all tests
finish.

[nhiroki, 2017-02-16 07:32:52]

On 2017/02/16 07:25:03, falken wrote:
> Nice bug. Does it make sense to commit the test to external/wpt instead?

This tests chromium-specific behavior (the HTML spec does not define behavior of
a detached frame), so out of external/wpt would be appropriate. Other worklet
tests should be upstreamed maybe when implementation gets slightly more mature.
I'll make a crbug for that.

[nhiroki, 2017-02-16 07:33:07]

Thank you! Updated.

https://codereview.chromium.org/2697243003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/worklet/import-on-detached-iframe.html
(right):

https://codereview.chromium.org/2697243003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/worklet/import-on-detached-iframe.html:12:
var frame = document.createElement('iframe');
On 2017/02/16 07:25:03, falken wrote:
> nit: let or const

Done.

https://codereview.chromium.org/2697243003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/worklet/import-on-detached-iframe.html:16:
add_completion_callback(() => frame.remove());
On 2017/02/16 07:25:03, falken wrote:
> nit: add_result_callback may be slightly cleaner in case more tests are added
to
> this file. If I understand correctly add_result_callback is invoked when this
> promise_test finishes, and add_completion_callback is invoked when all tests
> finish.

Yeah, it looks like add_result_callback is more preferable here. Replaced.

[nhiroki, 2017-02-16 07:33:41]

The CQ bit was checked by nhiroki@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 07:34:41]

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

[falken, 2017-02-16 07:35:57]

lgtm. please add a comment for "This tests chromium-specific behavior (the HTML
spec does not define behavior of
a detached frame), so out of external/wpt would be appropriate" and consider
putting it in a chromium/ directory to document that the test shouldn't be
upstreamed.

[nhiroki, 2017-02-16 08:47:36]

On 2017/02/16 07:35:57, falken wrote:
> lgtm. please add a comment for "This tests chromium-specific behavior (the
HTML
> spec does not define behavior of
> a detached frame), so out of external/wpt would be appropriate" and consider
> putting it in a chromium/ directory to document that the test shouldn't be
> upstreamed.

Yeah, I should have done this. Moved these to chromium/ and added comments.

[nhiroki, 2017-02-16 08:47:57]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2017-02-16 08:47:58]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/2697243003/#ps60001
(title: "move to chromium/")

[commit-bot: I haz the power, 2017-02-16 08:48:54]

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-02-16 11:50:00]

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

[commit-bot: I haz the power, 2017-02-16 11:50:02]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[nhiroki, 2017-02-16 13:46:00]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2017-02-16 13:46:17]

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-02-16 15:31:00]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487252760776020,
"parent_rev": "f2647997874eead6de0e93308011b55b6ae9160d", "commit_rev":
"5989961e2951701ff4a2d74bde290a0d26b45370"}

[commit-bot: I haz the power, 2017-02-16 15:31:40]

Description was changed from

==========
Worklet: Avoid import() on a detached frame

Without this fix, import() on a detached frame eventually crashes because of
a null execution context.

BUG=689265
==========

to

==========
Worklet: Avoid import() on a detached frame

Without this fix, import() on a detached frame eventually crashes because of
a null execution context.

BUG=689265

Review-Url: https://codereview.chromium.org/2697243003
Cr-Commit-Position: refs/heads/master@{#450972}
Committed:
https://chromium.googlesource.com/chromium/src/+/5989961e2951701ff4a2d74bde29...
==========

[commit-bot: I haz the power, 2017-02-16 15:31:42]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/5989961e2951701ff4a2d74bde29...