Blink: ExceptionState does not correctly clear exceptions |
|||
Issue descriptionVersion: Chromium 5f9ae975c6d36883e44ff5e43127abea282b15d5 V8 b34e6ff6c7fd084bf343d8c9b5bec76117d047a3 In Blink, ExceptionState::clearException() does not clear the Exception in V8. This can lead to problems. For instance, the following can happen: f() { // f() is a blink API function. An exception is thrown in here, // and the promise is rejected through ExceptionState::reject(), // which internally calls ExceptionState::clearException(). // Note that the exception is NOT cleared within V8. } try { f().then(function(value) { // fulfillment // Never reached }, function(reason) { // rejection // The expected code path, since the Promise was rejected. // However... }); } catch (e) { // This is reached instead, since the exception was never // cleared within V8. } A more detailed timeline: ------------------------- 1) f() is called through HandleApiCallHelper [0] 2) The exception is thrown. Since we are within an API call, it is turned into a scheduled exception [1] 3) ExceptionState::reject() is called, which in turn calls ExceptionState::clearException() [2] 4) clearException() does NOT clear V8 exceptions but only clears its member variable. 5) The scheduled exception is turned back into a real exception just before exiting HandleApiCallHelper [3] The reason this has worked so far is completely incidental. With the current V8 codebase, promise rejection eventually calls v8::internal::Script::GetNameOrSourceURL() [4], which first calls Execution::TryCall (promoting the scheduled exception), and then swallows the exception and returns undefined - and therefore step 5) has no exception to promote. To recap: --------- There are two issues here. First, V8 swallows scheduled exceptions during promise rejection. Second, blink does not correctly clear exceptions in ExceptionState::clearException. I'm working on a CL which removes Execution::TryCall from Script::GetNameOrSourceURL. This fixes the issue on the V8 side and exposes the blink clearException issue. ExceptionState::clearException() should be fixed in blink. How to reproduce: ----------------- * Check out chromium and v8 as specified above. * Apply patch [5] to V8, compile. * Run the attached test case: content_shell --run-layout-test ./third_party/WebKit/LayoutTests/storage/quota/storagequota-query-info.html * Alternatively run the original failing layout test: content_shell --run-layout-test ./third_party/WebKit/LayoutTests/storage/quota/storagequota-query-info.html [0] https://cs.chromium.org/chromium/src/v8/src/builtins/builtins.cc?q=HandleApiCallH&sq=package:chromium&l=5635 [1] https://cs.chromium.org/chromium/src/v8/src/isolate.cc?q=ScheduleThrow&sq=package:chromium&l=1312 [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/ExceptionState.cpp?q=ExceptionState::reject&sq=package:chromium&l=53 [3] https://cs.chromium.org/chromium/src/v8/src/builtins/builtins.cc?q=HandleApiCallH&sq=package:chromium&l=5705 [4] https://cs.chromium.org/chromium/src/v8/src/objects.cc?q=GetNameOrSou&sq=package:chromium&l=12822 [5] diff --git a/src/objects.cc b/src/objects.cc index b64ae2c..288c145 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -12830,22 +12830,13 @@ int Script::GetLineNumber(int code_pos) { Handle<Object> Script::GetNameOrSourceURL(Handle<Script> script) { Isolate* isolate = script->GetIsolate(); - Handle<String> name_or_source_url_key = - isolate->factory()->InternalizeOneByteString( - STATIC_CHAR_VECTOR("nameOrSourceURL")); - Handle<JSObject> script_wrapper = Script::GetWrapper(script); - Handle<Object> property = - JSReceiver::GetProperty(script_wrapper, name_or_source_url_key) - .ToHandleChecked(); - DCHECK(property->IsJSFunction()); - Handle<Object> result; - // Do not check against pending exception, since this function may be called - // when an exception has already been pending. - if (!Execution::TryCall(isolate, property, script_wrapper, 0, NULL) - .ToHandle(&result)) { - return isolate->factory()->undefined_value(); + + if (!script->source_url()->IsUndefined(isolate)) { + return handle(script->source_url(), isolate); } - return result; + return handle(script->name(), isolate); }
,
Jul 15 2016
ExceptionState::clearException is not supposed to clear the exception within v8. Where in this example is the exception coming from?
,
Jul 15 2016
The exception comes from the embedder. The embedder code throws an exception, but later converts it into a promise reject. However, the conversion does not clear the initial exception, and we end up with a scheduled exception as well as a rejected promise. Up until now the scheduled exception is swalled because on promise reject we do a TryCall. That does not happen anymore, and the scheduled exception is promoted later on. That causes the test failure. Jakob should have more details on when the exception is thrown from the embedder.
,
Jul 15 2016
specifically, I don't think that in the test you attached a promise is created. instead, we should bail out here: https://cs.chromium.org/chromium/src/out/Debug/gen/blink/bindings/modules/v8/V8StorageQuota.cpp?rcl=1468538294&l=69
,
Jul 15 2016
This is the backtrace at the throw site.
#0 v8::internal::Isolate::Throw (this=0x317e81b4f020, exception=0x31320cd96f09, location=0x0) at ../../v8/src/isolate.cc:1053
#1 0x00007fbdf0f7a5d3 in v8::internal::Isolate::ScheduleThrow (this=0x317e81b4f020, exception=0x31320cd96f09)
at ../../v8/src/isolate.cc:1315
#2 0x00007fbdf087697c in v8::Isolate::ThrowException (this=0x317e81b4f020, value=...) at ../../v8/src/api.cc:7300
#3 0x00007fbdeb92b3a9 in blink::V8ThrowException::throwException (exception=..., isolate=0x317e81b4f020)
at ../../third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:159
#4 0x00007fbdeb86f58f in blink::ExceptionState::throwException (this=0x7fbdc6cc8d10)
at ../../third_party/WebKit/Source/bindings/core/v8/ExceptionState.cpp:100
#5 0x00007fbdea36c703 in blink::ExceptionState::throwIfNeeded (this=0x7fbdc6cc8d10)
at ../../third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:99
#6 0x00007fbdea466c00 in blink::StorageQuotaV8Internal::queryInfoMethodPromise (info=..., exceptionState=...)
at gen/blink/bindings/modules/v8/V8StorageQuota.cpp:69
#7 0x00007fbdea46685c in blink::StorageQuotaV8Internal::queryInfoMethod (info=...) at gen/blink/bindings/modules/v8/V8StorageQuota.cpp:81
#8 0x00007fbdea4667a5 in blink::StorageQuotaV8Internal::queryInfoMethodCallback (info=...)
at gen/blink/bindings/modules/v8/V8StorageQuota.cpp:88
#9 0x00007fbdf086458a in v8::internal::FunctionCallbackArguments::Call (this=0x7fbdc6cc9068,
f=0x7fbdea466790 <blink::StorageQuotaV8Internal::queryInfoMethodCallback(v8::FunctionCallbackInfo<v8::Value> const&)>)
at ../../v8/src/api-arguments.cc:19
#10 0x00007fbdf09633ff in v8::internal::(anonymous namespace)::dis 2<false> (isolate=0x317e81b4f020, function=...,
new_target=..., fun_data=..., receiver=..., args=...) at ../../v8/src/builtins/builtins.cc:5702
#11 0x00007fbdf09a2f32 in v8::internal::Builtin_Impl_HandleApiCall (args=..., isolate=0x317e81b4f020)
at ../../v8/src/builtins/builtins.cc:5731
#12 0x00007fbdf096707d in v8::internal::Builtin_HandleApiCall (args_length=5, args_object=0x7fbdc6cc9310, isolate=0x317e81b4f020)
at ../../v8/src/builtins/builtins.cc:5719
Frame #7 then calls exceptionState.reject().
,
Jul 15 2016
ah, interesting. The bug is that we invoke throwIfNeeded().
There's a missing {% if not method.returns_promise %} {% endif %} around the throwIfNeeded() here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/templates/methods.cpp?rcl=0&l=211
,
Jul 18 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/894f497bb8a3bf00a32a396abae1a6e91824867d commit 894f497bb8a3bf00a32a396abae1a6e91824867d Author: jgruber <jgruber@chromium.org> Date: Mon Jul 18 08:53:20 2016 Check for method.returns_promise around ExceptionState::throwIfNeeded This ensures that either an exception is thrown, or a promise is rejected, never both. BUG= 628526 Review-Url: https://codereview.chromium.org/2156583002 Cr-Commit-Position: refs/heads/master@{#405967} [modify] https://crrev.com/894f497bb8a3bf00a32a396abae1a6e91824867d/third_party/WebKit/Source/bindings/templates/methods.cpp
,
Jul 18 2016
|
|||
►
Sign in to add a comment |
|||
Comment 1 by yangguo@chromium.org
, Jul 15 2016