New issue
Advanced search Search tips

Issue 862440 link

Starred by 6 users
Status: Fixed
Owner:
yhirano@chromium.org
Closed: Jul 12
Cc:
falken@chromium.org
yukishiino@chromium.org
jkummerow@chromium.org
cbruni@chromium.org
ricea@chromium.org
haraken@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

BodyStreamBuffer::Stream() returns empty handle, causing crashes

Project Member Reported by jkummerow@chromium.org, Jul 10 
Chrome Version       : 69.0.3487
OS Version: all
URLs (if applicable) : 

After https://chromium-review.googlesource.com/c/chromium/src/+/1124270, BodyStreamBuffer::Stream() sometimes(?) returns a ScriptValue whose value_.IsEmpty(). This causes nullptr-deref crashes when such a ScriptValue is passed as a function argument to V8; that happens for example in:
- BodyStreamBuffer::CloseAndLockAndDisturb
- BodyStreamBuffer::BooleanStreamOperation
and possibly other places.

To repro: open https://material-ui.com/ and wait a few seconds. The ServiceWorker thread will crash with a segfault.

On the current Canary, this is the #1 renderer crasher on all platforms: 

https://crash.corp.google.com/browse?q=product_name%3D%27Chrome%27+AND+product.version%3D%2769.0.3487.0%27+AND+expanded_custom_data.ChromeCrashProto.channel%3D%27canary%27+AND+expanded_custom_data.ChromeCrashProto.ptype%3D%27renderer%27+AND+expanded_custom_data.ChromeCrashProto.magic_signature_1.name%3D%27v8%3A%3Ainternal%3A%3A%60anonymous+namespace%5C%27%3A%3AInvoke%27#-productname:1000,-magicsignature:50,-magicsignature2:50,-stablesignature:50,crashreason,crashaddress,processuptime,url:20,-magicsignaturesorted:50

Comment 1 by yhirano@chromium.org, Jul 11 

I'm reverting the CL.

Comment 2 by ajha@chromium.org, Jul 11

Labels: M-69
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 11 

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

commit 7a0a490dd3d9d640e088aee823bd069c74a8ef64
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Wed Jul 11 03:51:54 2018

Revert "[Fetch] Use wrapper tracing to express references between wrappers"

This reverts commit 8057f58cb22579bc685efa50c579325ed047d51a.

Reason for revert: Crashing. See  https://crbug.com/862440 .

Original change's description:
> [Fetch] Use wrapper tracing to express references between wrappers
>
> BodyStreamBuffer has a ReadableStream instance, but because it's
> implemented in V8Extra we cannot holds it as a member (i.e., as a
> ScriptValue). Instead, we attach the value to the JS wrapper of the
> C++ object. This required us to maintain chains of wrappers,
> FetchEvent -> Request -> BodyStreamBuffer for example, manually.
>
> This CL replaces that mechanism with wrapper tracing.
>
> Bug: None
> Change-Id: I889ef0d0442d62ad50826a4b487ec234f041a982
> Reviewed-on: https://chromium-review.googlesource.com/1124270
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Adam Rice <ricea@chromium.org>
> Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573533}

TBR=falken@chromium.org,ricea@chromium.org,yukishiino@chromium.org,yhirano@chromium.org,haraken@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  862440 
Change-Id: I4c1bd85ff5f4b078b4c0c56474e279fe83fc5eaa
Reviewed-on: https://chromium-review.googlesource.com/1132634
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574065}
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/core/fetch/body_stream_buffer.cc
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/core/fetch/body_stream_buffer.h
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/core/fetch/fetch_request_data.cc
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/core/fetch/fetch_request_data.h
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/core/fetch/fetch_response_data.cc
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/core/fetch/fetch_response_data.h
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/core/fetch/request.cc
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/core/fetch/request.h
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/core/fetch/response.cc
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/core/fetch/response.h
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/modules/service_worker/fetch_event.cc
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/modules/service_worker/fetch_event.h
[modify] https://crrev.com/7a0a490dd3d9d640e088aee823bd069c74a8ef64/third_party/blink/renderer/platform/bindings/v8_private_property.h

Comment 4 by ajha@chromium.org, Jul 11

Labels: OS-Android
Top crasher on Android canary version 69.0.3487.0 as well. 964 crashes from 560 client ids so far.
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 11

Labels: merge-merged-3488
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b1bd2a01611eb215e3eda29499131262f45ea66

commit 1b1bd2a01611eb215e3eda29499131262f45ea66
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Wed Jul 11 17:37:26 2018

Revert "[Fetch] Use wrapper tracing to express references between wrappers"

This reverts commit 8057f58cb22579bc685efa50c579325ed047d51a.

Reason for revert: Crashing. See  https://crbug.com/862440 .

Original change's description:
> [Fetch] Use wrapper tracing to express references between wrappers
>
> BodyStreamBuffer has a ReadableStream instance, but because it's
> implemented in V8Extra we cannot holds it as a member (i.e., as a
> ScriptValue). Instead, we attach the value to the JS wrapper of the
> C++ object. This required us to maintain chains of wrappers,
> FetchEvent -> Request -> BodyStreamBuffer for example, manually.
>
> This CL replaces that mechanism with wrapper tracing.
>
> Bug: None
> Change-Id: I889ef0d0442d62ad50826a4b487ec234f041a982
> Reviewed-on: https://chromium-review.googlesource.com/1124270
> Reviewed-by: Kentaro Hara <haraken@chromium.org>
> Reviewed-by: Adam Rice <ricea@chromium.org>
> Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573533}

TBR=falken@chromium.org,ricea@chromium.org,yukishiino@chromium.org,yhirano@chromium.org,haraken@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  862440 
Change-Id: I4c1bd85ff5f4b078b4c0c56474e279fe83fc5eaa
Reviewed-on: https://chromium-review.googlesource.com/1132634
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#574065}(cherry picked from commit 7a0a490dd3d9d640e088aee823bd069c74a8ef64)
Reviewed-on: https://chromium-review.googlesource.com/1133341
Reviewed-by: Ben Mason <benmason@chromium.org>
Cr-Commit-Position: refs/branch-heads/3488@{#3}
Cr-Branched-From: 426a53b7a3cf88a3c24a61d14346a6815ee2b2d4-refs/heads/master@{#574034}
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/core/fetch/body_stream_buffer.cc
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/core/fetch/body_stream_buffer.h
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/core/fetch/fetch_request_data.cc
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/core/fetch/fetch_request_data.h
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/core/fetch/fetch_response_data.cc
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/core/fetch/fetch_response_data.h
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/core/fetch/request.cc
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/core/fetch/request.h
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/core/fetch/response.cc
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/core/fetch/response.h
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/modules/service_worker/fetch_event.cc
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/modules/service_worker/fetch_event.h
[modify] https://crrev.com/1b1bd2a01611eb215e3eda29499131262f45ea66/third_party/blink/renderer/platform/bindings/v8_private_property.h

Comment 6 by yhirano@chromium.org, Jul 12 

Issue 861898 has been merged into this issue.

Comment 7 by yhirano@chromium.org, Jul 12

Status: Fixed (was: Assigned)

Comment 8 by ajha@chromium.org, Jul 12

Labels: TE-Verified-M69 TE-Verified-69.0.3488.2
Windows,Mac canary version: 69.0.3489.0 has been live for 5 hrs and spike seems to have recovered there.


No such spike is seen on Android canary version: 69.0.3488.2 and 69.0.3489.0 as well.
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 12 

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

commit 4a7c8ee607674efabc720016992893e2226ce3a0
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Thu Jul 12 11:53:46 2018

[Fetch] Use wrapper tracing to express references between wrappers

This is a reland of
https://crrev.com/8057f58cb22579bc685efa50c579325ed047d51a which has
been reverted due to a crash issue. The crash was caused by a missing
TraceWrapperMember in blink::Response. This CL fixes that, and adds
a layout test to verify that.

Original CL: https://chromium-review.googlesource.com/c/1124270

Bug:  862440 
Change-Id: I2842b957f223cfb32fee2aa9048b550010cb2e45
Reviewed-on: https://chromium-review.googlesource.com/1134642
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574527}
[add] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/WebKit/LayoutTests/http/tests/fetch/chromium/body-wrapper-tracing.html
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/core/fetch/body_stream_buffer.cc
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/core/fetch/body_stream_buffer.h
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/core/fetch/fetch_request_data.cc
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/core/fetch/fetch_request_data.h
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/core/fetch/fetch_response_data.cc
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/core/fetch/fetch_response_data.h
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/core/fetch/request.cc
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/core/fetch/request.h
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/core/fetch/response.cc
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/core/fetch/response.h
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/modules/service_worker/fetch_event.cc
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/modules/service_worker/fetch_event.h
[modify] https://crrev.com/4a7c8ee607674efabc720016992893e2226ce3a0/third_party/blink/renderer/platform/bindings/v8_private_property.h

Comment 10 by ricea@chromium.org, Aug 10

Cc: jkummerow@chromium.org cbruni@chromium.org
 Issue 872653  has been merged into this issue.

Comment 11 by ricea@chromium.org, Aug 10 

Issue 862394 has been merged into this issue.
►

Sign in to add a comment