New issue
Advanced search Search tips

Issue 879366 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 31
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

blink::Document::open() can crash

Project Member Reported by timothygu@chromium.org, Aug 30

Issue description

Chrome Version: (copy from chrome://version) 70.0.3538.0 (Developer Build) (64-bit)
OS: (e.g. Win10, MacOS 10.12, etc...) Linux

Also reproduced on:
- 68.0.3440.106 (Official Build) (64-bit) on Linux
- 70.0.3534.4 (Official Build) dev (64-bit) on macOS 10.13.6
- 70.0.3537.0 (Official Build) canary (64-bit) on macOS 10.13.6

What steps will reproduce the problem?

<body>
<script>
const div = document.body.appendChild(document.createElement("div"));
div.innerHTML = "<iframe src='data:text/html,'></iframe>";
const frame = div.childNodes[0];
const client = new frame.contentWindow.XMLHttpRequest();
client.open("GET", "data:text/html,");
client.onabort = e => {
  div.remove();
};
client.send();
frame.contentWindow.document.open();
</script>


What is the expected result?

Renderer process doesn't crash.

What happens instead?

Renderer process crashes with the following backtrace:

Received signal 11 SEGV_MAPERR 000000000078
#0 0x7fadf19d8bed base::debug::StackTrace::StackTrace()
#1 0x7fadf16df9ac base::debug::StackTrace::StackTrace()
#2 0x7fadf19d8644 base::debug::(anonymous namespace)::StackDumpSignalHandler()
#3 0x7fadcb9320c0 <unknown>
#4 0x7fadd3b48cac logging::CheckOpResult::message()
#5 0x7fadd3bf8e59 blink::Page::MainFrame()
#6 0x7fadd462fba5 blink::LocalFrame::Client()
#7 0x7fadd4193780 blink::Document::open()
#8 0x7fadd419de22 blink::Document::open()
#9 0x7fadd419e8a0 blink::Document::open()
#10 0x7fadd571226a blink::DocumentV8Internal::open1Method()
#11 0x7fadd570652b blink::DocumentV8Internal::openMethod()
#12 0x7fadd570641a blink::V8Document::openMethodCallback()
#13 0x7fadd66baf04 v8::internal::FunctionCallbackArguments::Call()
#14 0x7fadd66b95d1 v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#15 0x7fadd66b7a18 v8::internal::Builtin_Impl_HandleApiCall()
#16 0x7fadd66b74b1 v8::internal::Builtin_HandleApiCall()
#17 0x7fadd7403c95 <unknown>
  r8: 00000000fffffff7  r9: 0000000000000000 r10: 0000000000000000 r11: 00007fadc7805e00
 r12: 00007ffe16ab0a10 r13: 00007fadd5706400 r14: 00007ffe16ab0a98 r15: 000030218c6c8020
  di: 0000000000000078  si: 000011a4957fd7f8  bp: 00007ffe16aaff20  bx: 00007fadf1a8e6a0
  dx: 3b13a52003b4e100  ax: 0000000000000000  cx: 3b13a52003b4e100  sp: 00007ffe16aaff20
  ip: 00007fadd3b48cac efl: 0000000000010206 cgf: 002b000000000033 erf: 0000000000000004
 trp: 000000000000000e msk: 0000000000000000 cr2: 0000000000000078
[end of stack trace]
Calling _exit(1). Core file will not be generated.

Please use labels and text to provide additional information.

If this is a regression (i.e., worked before), please consider using the
bisect tool (https://www.chromium.org/developers/bisect-builds-py) to help
us identify the root cause and more rapidly triage the issue.

For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.


 
Status: Fixed (was: Started)
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 31

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

commit 09b4427ec034d1e7e89ab1508c099a7c4411b65c
Author: Timothy Gu <timothygu@chromium.org>
Date: Fri Aug 31 03:24:17 2018

document.open(): Check frame_ after StopAllLoaders

FrameLoader::StopAllLoaders() has this explicit note:

    Warning: stopAllLoaders can and will detach the LocalFrame out from
    under you. All callers need to either protect the LocalFrame or
    guarantee they won't in any way access the LocalFrame after
    stopAllLoaders returns.

Check frame_'s existence after the call to prevent a NULL dereference.

Bug:  879366 
Change-Id: I1e537374f59fbad7b069f9de63cfa3b6b2b2b00c
Reviewed-on: https://chromium-review.googlesource.com/1198022
Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587933}
[add] https://crrev.com/09b4427ec034d1e7e89ab1508c099a7c4411b65c/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2-expected.txt
[add] https://crrev.com/09b4427ec034d1e7e89ab1508c099a7c4411b65c/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2.html
[modify] https://crrev.com/09b4427ec034d1e7e89ab1508c099a7c4411b65c/third_party/blink/renderer/core/dom/document.cc

Labels: Merge-Request-70 M-70
Project Member

Comment 4 by sheriffbot@chromium.org, Sep 5

Labels: -Merge-Request-70 Hotlist-Merge-Approved Merge-Approved-70
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 5

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1f0040675e06401a05924d6187186ce08649b925

commit 1f0040675e06401a05924d6187186ce08649b925
Author: Timothy Gu <timothygu@chromium.org>
Date: Wed Sep 05 18:45:49 2018

document.open(): Check frame_ after StopAllLoaders

FrameLoader::StopAllLoaders() has this explicit note:

    Warning: stopAllLoaders can and will detach the LocalFrame out from
    under you. All callers need to either protect the LocalFrame or
    guarantee they won't in any way access the LocalFrame after
    stopAllLoaders returns.

Check frame_'s existence after the call to prevent a NULL dereference.

Bug:  879366 
Change-Id: I1e537374f59fbad7b069f9de63cfa3b6b2b2b00c
Reviewed-on: https://chromium-review.googlesource.com/1198022
Reviewed-by: Nate Chapin <japhet@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#587933}(cherry picked from commit 09b4427ec034d1e7e89ab1508c099a7c4411b65c)
Reviewed-on: https://chromium-review.googlesource.com/1207612
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#61}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[add] https://crrev.com/1f0040675e06401a05924d6187186ce08649b925/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2-expected.txt
[add] https://crrev.com/1f0040675e06401a05924d6187186ce08649b925/third_party/WebKit/LayoutTests/fast/dom/Document/open-with-pending-load2.html
[modify] https://crrev.com/1f0040675e06401a05924d6187186ce08649b925/third_party/blink/renderer/core/dom/document.cc

Sign in to add a comment