Repro wrapper in HTML for your convenience.

Deleted: repro.html 53 bytes

repro.html 53 bytes View Download

Crash ID - 7e12b63500000000

Stack Trace:
============

Thread 0 CRASHED [EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000000 ] MAGIC SIGNATURE THREAD
0x57882fe9	(chrome_child.dll -scriptwrappable.h:107 )	blink::ScriptWrappable::setWrapper(v8::Isolate *,blink::WrapperTypeInfo const *,v8::Local<v8::Object> &)
0x57882e48	(chrome_child.dll -v8domwrapper.h:90 )	blink::V8DOMWrapper::associateObjectWithWrapper(v8::Isolate *,blink::ScriptWrappable *,blink::WrapperTypeInfo const *,v8::Local<v8::Object>)
0x578f89aa	(chrome_child.dll -scriptwrappable.cpp:34 )	blink::ScriptWrappable::associateWithWrapper(v8::Isolate *,blink::WrapperTypeInfo const *,v8::Local<v8::Object>)
0x578f87ba	(chrome_child.dll -scriptwrappable.cpp:29 )	blink::ScriptWrappable::wrap(v8::Isolate *,v8::Local<v8::Object>)
0x578f86e4	(chrome_child.dll -tov8.h:40 )	blink::toV8(blink::ScriptWrappable *,v8::Local<v8::Object>,v8::Isolate *)
0x579768f0	(chrome_child.dll -v8binding.h:347 )	blink::v8SetReturnValueFast<v8::FunctionCallbackInfo<v8::Value> >(v8::FunctionCallbackInfo<v8::Value> const &,blink::EventTarget *,blink::ScriptWrappable const *)
0x58154c34	(chrome_child.dll -v8element.cpp:2201 )	blink::ElementV8Internal::animate1Method
0x5815515d	(chrome_child.dll -v8element.cpp:2252 )	blink::ElementV8Internal::animateMethod
0x5786cbf0	(chrome_child.dll -api-arguments.cc:21 )	v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const &))
0x5786c8fa	(chrome_child.dll -builtins-api.cc:106 )	v8::internal::`anonymous namespace'::HandleApiCallHelper<0>
0x5786c6d7	(chrome_child.dll -builtins-api.cc:135 )	v8::internal::Builtin_Impl_HandleApiCall
0x5786c5f2	(chrome_child.dll -builtins-api.cc:123 )	v8::internal::Builtin_HandleApiCall(int,v8::internal::Object * *,v8::internal::Isolate *)
0x577eacc6	(chrome_child.dll -execution.cc:139 )	v8::internal::`anonymous namespace'::Invoke
0x578faa72	(chrome_child.dll -v8scriptrunner.cpp:511 )	blink::V8ScriptRunner::callFunction(v8::Local<v8::Function>,blink::ExecutionContext *,v8::Local<v8::Value>,int,v8::Local<v8::Value> * const,v8::Isolate *)

Crash observed on the following build:

https://crash.corp.google.com/browse?q=product.name%3D%27Chrome%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27renderer%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27blink%3A%3AScriptWrappable%3A%3AsetWrapper%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D#samplereports:5,productversion:1000

====================================

Good Build:

54.0.2800.0    Base Position: 406166


Bad Build:

54.0.2824.0    Base Position: 410520

=====================================

Able to repro this issue on Windows 7, MAC (10.11.6) & Ubuntu Trusty (14.04) for the Google Chrome Stable Version - 53.0.2785.101

This is a regression issue broken in M54, below mentioned is the bisect info:

CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/6b25a0aa041c31e2ebd85bb308672809fc7d940a..ec8f330ece20d7324d649dea227dd4178b2b7b64

Suspecting Commit: 120fd684181b5730f243f87189d498ce5f0a1f8d 

Review URL: https://codereview.chromium.org/2209593002

@haraken: Could you please look into the issue, and if it has nothing to do with your changes and if possible please do assign it to the concerned owner.

Thank you.

Users experienced this crash on the following builds:

Win Canary 56.0.2905.0 -  0.07 CPM, 3 reports, 3 clients (signature blink::ScriptWrappable::setWrapper)

If this update was incorrect, please add "Fracas-Wrong" label to prevent future updates.

- Go/Fracas

ping - haraken@ bisect seems likely correct.  PTAL.  Worth considering a revert?

yukishiino@: Do you have any idea on this? toV8() is returning an empty handle for the test case attached in #0... I guess this is not an OOM.

The root cause was that Dictionary does not handle a possible exception when retrieving a dictionary member.  Dictionary member is a property, that can be an accessor property, that can do anything including throwing an exception.

    dictionary.get(key, &value);  // throws an exception.
    toV8(obj);  // fails because V8 APIs don't work with an exception being thrown.

I prepared a fix at http://crrev.com/2496533002 .

This is a top crash in the latest Android dev release (https://crash.corp.google.com/browse?q=product.name%3D%27Chrome_Android%27%20AND%20product.version%3D%2756.0.2913.4%27%20AND%20custom_data.ChromeCrashProto.ptype%3D%27renderer%27%20AND%20custom_data.ChromeCrashProto.magic_signature_1.name%3D%27v8%3A%3Ainternal%3A%3AGlobalHandles%3A%3AMakeWeak%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D).  Please ensure the fix is landed by Monday.

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

commit 5f971b7bafe1c9a8574e66525b593644e4b9d658
Author: yukishiino <yukishiino@chromium.org>
Date: Mon Nov 14 10:05:29 2016

binding: Makes Dictionary handle a possible exception in [[Get]].

Dictionary retrieves a dictionary member through the [[Get]] internal
method of the dictionary object.  Since the properties can be accessor
properties, arbitrary script may run in [[Get]], so we have to handle
a possible exception in [[Get]].

The right fix is to propagate the exception up to the caller,
however, there are so many callers and the issue is urgent.
So simply swallowing all the exceptions in this CL, and fix it
re-throws in a follow-up CL.

BUG= 644237 

Review-Url: https://codereview.chromium.org/2496533002
Cr-Commit-Position: refs/heads/master@{#431848}

[add] https://crrev.com/5f971b7bafe1c9a8574e66525b593644e4b9d658/third_party/WebKit/LayoutTests/fast/dom/dictionary-member-get-throws.html
[modify] https://crrev.com/5f971b7bafe1c9a8574e66525b593644e4b9d658/third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/KeyframeEffect/constructor-expected.txt
[modify] https://crrev.com/5f971b7bafe1c9a8574e66525b593644e4b9d658/third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp
[modify] https://crrev.com/5f971b7bafe1c9a8574e66525b593644e4b9d658/third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp

 Issue 660818  has been merged into this issue.

Seeing Issue 667298, this issue may be happening in M54 and M55, too.  May I request a merge to M54 and M55?

[Automated comment] Request affecting a post-stable build (M54), manual review required.

[Automated comment] Less than 2 weeks to go before stable on M55, manual review required.

Please find the update of the crash on Chrome Desktop below : 

Chrome Windows : Crash was last seen on Chrome version 51.0.2704.103(https://goto.google.com/daloib)

Chrome Mac : Crash was last seen on Chrome version 56.0.2923.0(https://goto.google.com/mpnoe)

If we consider bug in comment#12 and query the Chromecrash below are the Chrome version where this crash was seen on Desktop Chrome :

Chrome Windows : Crash was last seen on Chrome version 56.0.2913.0(https://goto.google.com/jzfyk)

Chrome Mac : Crash was last seen on Chrome 56.0.2910.0(https://goto.google.com/xopnn)

Thank you pbommana@.

yukishiino@, before we approve merge to M55, could you please confirm this change is well baked/verified in Canary/Dev and safe to merge?

The patch landed on Nov 14th, and since then, I don't see any problem with Canary/Dev/ToT.  I think it's safe.

Approving merge to M55 branch 2883 based on comment #17. Please merge ASAP. Please merge before 5:00 PM PT, Friday (11/25) in order into make to Desktop final build cut. Thank you.

[Automated comment] There appears to be on-going work (i.e. bugroid changes), needs manual review.

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

commit f924a45a1c9276a2f145ece28b6d645839997981
Author: Yuki Shiino <yukishiino@chromium.org>
Date: Fri Nov 25 06:03:42 2016

binding: Makes Dictionary handle a possible exception in [[Get]].

Dictionary retrieves a dictionary member through the [[Get]] internal
method of the dictionary object.  Since the properties can be accessor
properties, arbitrary script may run in [[Get]], so we have to handle
a possible exception in [[Get]].

The right fix is to propagate the exception up to the caller,
however, there are so many callers and the issue is urgent.
So simply swallowing all the exceptions in this CL, and fix it
re-throws in a follow-up CL.

BUG= 644237 

Review-Url: https://codereview.chromium.org/2496533002
Cr-Commit-Position: refs/heads/master@{#431848}
(cherry picked from commit 5f971b7bafe1c9a8574e66525b593644e4b9d658)

Review URL: https://codereview.chromium.org/2529033002 .

Cr-Commit-Position: refs/branch-heads/2883@{#660}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[add] https://crrev.com/f924a45a1c9276a2f145ece28b6d645839997981/third_party/WebKit/LayoutTests/fast/dom/dictionary-member-get-throws.html
[modify] https://crrev.com/f924a45a1c9276a2f145ece28b6d645839997981/third_party/WebKit/LayoutTests/imported/wpt/web-animations/interfaces/KeyframeEffect/constructor-expected.txt
[modify] https://crrev.com/f924a45a1c9276a2f145ece28b6d645839997981/third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp
[modify] https://crrev.com/f924a45a1c9276a2f145ece28b6d645839997981/third_party/WebKit/Source/bindings/core/v8/DictionaryIterator.cpp

I've done the merge to M55(2883).

Verified the issue on Windows 10, Ubuntu 14.04 and Mac 10.11.6 using chrome latest beta M55-55.0.2883.70
Opened the "repro.html" provided in the comment #1 and no crash observed.

Hence adding TE-Verified label.

Attaching the screen-cast for your reference.

Deleted: Issue 644237.mp4 451 KB

	Issue 644237.mp4 451 KB View Download

No more M54s planned AFAIK.

re #23: Thanks for letting me know.  Then, I've done all what I can do.

Issue 667298 has been merged into this issue.