FrameView can be null in addTextMatchMaker() after appendChild(iframe)

FrameView can be null in addTextMatchMaker() after appendChild(iframe)

Range's owner document's FrameView can be cleared after the iframe
to which the range belongs to is appendChild()ed.
This CL fixes Internals::addTextMatchMarker() to handle such cases.

BUG=693848
Review-Url: https://codereview.chromium.org/2706913003
Cr-Commit-Position: refs/heads/master@{#451929}
Committed: https://chromium.googlesource.com/chromium/src/+/68ca487c1e84a61cbcf3995637376d45bc1ff01b

[hiroshige, 2017-02-21 22:37:33]

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

[commit-bot: I haz the power, 2017-02-21 22:38:37]

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

[hiroshige, 2017-02-21 22:39:51]

Description was changed from

==========
Fix crash in Internals::addTextMatchMaker()

BUG=693848
==========

to

==========
Fix crash in Internals::addTextMatchMaker()

Range's owner document's FrameView is cleared after the iframe to which
the range belongs to is appendChild()ed.
This CL fixes Internals::addTextMatchMarker() to handle such cases.

BUG=693848
==========

[hiroshige, 2017-02-21 22:46:50]

Description was changed from

==========
Fix crash in Internals::addTextMatchMaker()

Range's owner document's FrameView is cleared after the iframe to which
the range belongs to is appendChild()ed.
This CL fixes Internals::addTextMatchMarker() to handle such cases.

BUG=693848
==========

to

==========
FrameView can be null in addTextMatchMaker() after appendChild(iframe)

Range's owner document's FrameView can be cleared after the iframe
to which the range belongs to is appendChild()ed.
This CL fixes Internals::addTextMatchMarker() to handle such cases.

BUG=693848
==========

[hiroshige, 2017-02-21 22:53:19]

hiroshige@chromium.org changed reviewers:
+ blink-reviews-dom@chromium.org, tkent@chromium.org, wangxianzhu@chromium.org,
yosin@chromium.org

[hiroshige, 2017-02-21 22:53:19]

Could you take a look:

wangxianzhu@ as the reviewer of https://codereview.chromium.org/2450793002 that
introduced the crashing function call,

tkent@ yosin@, could you check whether anything wrong occurs outside
blink::Internals?
Is this expected behavior of appendChild()?

[Xianzhu, 2017-02-21 22:57:25]

LGTM.

I ever marked several similar bugs WontFix. In real world addTextMatchMaker is
never called in such a situation. It occurs because we expose it in internal API
so a specially written layout test can produce such a situation.

[tkent, 2017-02-21 23:19:45]

> tkent@ yosin@, could you check whether anything wrong occurs outside
blink::Internals?
> Is this expected behavior of appendChild()?

It's expected.  removeChild() inside appendChild() will detach the Document from
the IFRAME. The Document won't be associated to any Frames.

https://codereview.chromium.org/2706913003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/add-text-match-maker-crash.html
(right):

https://codereview.chromium.org/2706913003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/add-text-match-maker-crash.html:1:
<script src="../../resources/js-test.js"></script>
Please do not add new test using js-test.js.

IMO, we don't need to add a test for an Internals crash.

[yosin_UTC9, 2017-02-21 23:24:17]

Thanks for fixing this!

https://codereview.chromium.org/2706913003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2706913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.cpp:1050: if
(range->ownerDocument().view())
Could you do early return at top of function, e.g. L1044?
Since we don't need to update layout and update markers.

void Internals::addTextMatchMarker(const Range* range, bool isActive) {
  DCHECK(range);
  if (!range->ownerDocument().view())
   return;
  range->ownerDocument().updateStyleAndLayoutIgnorePendingStylesheets();

[hiroshige, 2017-02-21 23:26:17]

Thanks for reviewing!

Removed the test according to the review comment.

[hiroshige, 2017-02-21 23:29:57]

https://codereview.chromium.org/2706913003/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/paint/invalidation/add-text-match-maker-crash.html
(right):

https://codereview.chromium.org/2706913003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/invalidation/add-text-match-maker-crash.html:1:
<script src="../../resources/js-test.js"></script>
On 2017/02/21 23:19:44, tkent wrote:
> Please do not add new test using js-test.js.
> 
> IMO, we don't need to add a test for an Internals crash.

Removed.

https://codereview.chromium.org/2706913003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2706913003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.cpp:1050: if
(range->ownerDocument().view())
On 2017/02/21 23:24:17, yosin_UTC9 wrote:
> Could you do early return at top of function, e.g. L1044?
> Since we don't need to update layout and update markers.
> 
> void Internals::addTextMatchMarker(const Range* range, bool isActive) {
>   DCHECK(range);
>   if (!range->ownerDocument().view())
>    return;
>   range->ownerDocument().updateStyleAndLayoutIgnorePendingStylesheets();

Done.

[tkent, 2017-02-21 23:32:00]

lgtm

[yosin_UTC9, 2017-02-21 23:43:28]

lgtm

Thanks!

[hiroshige, 2017-02-21 23:50:19]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-02-21 23:50:19]

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

[commit-bot: I haz the power, 2017-02-21 23:52:42]

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-22 01:55:54]

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

[commit-bot: I haz the power, 2017-02-22 01:55:56]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[hiroshige, 2017-02-22 02:23:16]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-02-22 02:24:15]

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

[hiroshige, 2017-02-22 03:27:40]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-02-22 03:27:49]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2017-02-22 03:28: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-22 04:26:20]

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

[commit-bot: I haz the power, 2017-02-22 04:26:21]

Exceeded global retry quota

[Xianzhu, 2017-02-22 06:39:28]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-02-22 06:40:25]

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-22 08:29:27]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487745568717330,
"parent_rev": "d94ddbb3f5a97d20d6a4f568ce8a87515dc74e04", "commit_rev":
"68ca487c1e84a61cbcf3995637376d45bc1ff01b"}

[commit-bot: I haz the power, 2017-02-22 08:30:05]

Description was changed from

==========
FrameView can be null in addTextMatchMaker() after appendChild(iframe)

Range's owner document's FrameView can be cleared after the iframe
to which the range belongs to is appendChild()ed.
This CL fixes Internals::addTextMatchMarker() to handle such cases.

BUG=693848
==========

to

==========
FrameView can be null in addTextMatchMaker() after appendChild(iframe)

Range's owner document's FrameView can be cleared after the iframe
to which the range belongs to is appendChild()ed.
This CL fixes Internals::addTextMatchMarker() to handle such cases.

BUG=693848

Review-Url: https://codereview.chromium.org/2706913003
Cr-Commit-Position: refs/heads/master@{#451929}
Committed:
https://chromium.googlesource.com/chromium/src/+/68ca487c1e84a61cbcf399563737...
==========

[commit-bot: I haz the power, 2017-02-22 08:30:06]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/68ca487c1e84a61cbcf399563737...