Issue 2209203002: binding: Always throws an exception in the current realm.

Yuki

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

4 years, 4 months ago (2016-08-04 09:08:26 UTC) #1

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-04 09:08:42 UTC) #2

Yuki

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

4 years, 4 months ago (2016-08-04 09:45:28 UTC) #3

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-04 09:45:42 UTC) #4

Yuki

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

4 years, 4 months ago (2016-08-04 10:01:43 UTC) #5

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-04 10:01:53 UTC) #6

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-04 10:55:24 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/271635)

4 years, 4 months ago (2016-08-04 10:55:25 UTC) #8

Yuki

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

4 years, 4 months ago (2016-08-09 07:36:30 UTC) #9

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-09 07:37:16 UTC) #10

Yuki

yukishiino@chromium.org changed reviewers: + haraken@chromium.org

4 years, 4 months ago (2016-08-09 07:39:21 UTC) #11

haraken

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/LayoutTests/fast/dom/cross-frame-accessor-throw-expected.txt File third_party/WebKit/LayoutTests/fast/dom/cross-frame-accessor-throw-expected.txt (right): https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/LayoutTests/fast/dom/cross-frame-accessor-throw-expected.txt#newcode3 third_party/WebKit/LayoutTests/fast/dom/cross-frame-accessor-throw-expected.txt:3: FAIL Check that DOM exception is created in setter's ...

4 years, 4 months ago (2016-08-09 07:53:51 UTC) #13

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-09 10:02:16 UTC) #14

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/269710)

4 years, 4 months ago (2016-08-09 10:02:18 UTC) #15

Yuki

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/LayoutTests/fast/dom/cross-frame-accessor-throw-expected.txt File third_party/WebKit/LayoutTests/fast/dom/cross-frame-accessor-throw-expected.txt (right): https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/LayoutTests/fast/dom/cross-frame-accessor-throw-expected.txt#newcode3 third_party/WebKit/LayoutTests/fast/dom/cross-frame-accessor-throw-expected.txt:3: FAIL Check that DOM exception is created in setter's ...

4 years, 4 months ago (2016-08-10 03:11:10 UTC) #16

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/cross-frame-accessor-throw-expected.txt
(right):

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/cross-frame-accessor-throw-expected.txt:3:
FAIL Check that DOM exception is created in setter's context. assert_equals:
expected [stringifying object threw TypeError: Illegal invocation with type
object] but got [stringifying object threw TypeError: Illegal invocation with
type object]
On 2016/08/09 07:53:50, haraken wrote:
> 
> Is it okay to leave the FAIL line? I was expecting that this CL makes it pass.

This failure is caused by a V8 bug that V8 does not change the current realm if
LAP is called.  Once V8 fixes it, this test must become green.

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp (left):

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:73: if (!frame
|| !BindingSecurity::shouldAllowAccessToFrame(isolate,
currentDOMWindow(isolate), frame, DoNotReportSecurityError)) {
On 2016/08/09 07:53:50, haraken wrote:
> 
> Do you know why we had this security check? This CL makes sense but I want to
> make sure that we're not removing code that shouldn't be removed.

The old implementation was creating a new exception in the given context
(|creationContext|) so we need to check that the current realm has the right
access to that context.

Now, we always create an exception in the current realm, there is no need to
check.

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp (right):

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:54:
v8::Local<v8::Value> V8ThrowException::createDOMException(v8::Isolate* isolate,
ExceptionCode exceptionCode, const String& sanitizedMessage, const String&
unsanitizedMessage)
On 2016/08/09 07:53:50, haraken wrote:
> 
> Just to confirm: Is it guaranteed that the current implementation calls
> createDOMException with a correct current context?

I suppose so.  There are three use cases of v8::Context::Enter in Blink.
1. InjectedScript: They intentionally run the code in the specific context, so
it must be okay to throw in that context.
2. V8WrapperInstantiationScope: It carefully handles the cross-origin case, and
rethrows if necessary.
3. ScriptState::Scope: It must be okay to create an exception in the current
realm while ScriptState::Scope() is created on the stack.

In general, exceptions can be thrown in the current realm by V8, so we must be
prepared for an exception created in the current realm.  e.g.
v8::Exception::{Range,Reference,Syntax,Type,}Error have always been created in
the current realm.

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:82: if
(tryCatch.HasCaught()) {
On 2016/08/09 07:53:50, haraken wrote:
> 
> Just help me understand: Who can throw an exception?

OOM (out of handles or stack) in V8?

Probably the original author wanted to make it sure that this function *creates*
an exception, does not *throw*.

I didn't realize this unlikeliness.  May I remove this TryCatch block?

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:87: if
(!v8CallBoolean(exceptionObj->SetAccessor(isolate->GetCurrentContext(),
v8AtomicString(isolate, "stack"), domExceptionStackGetter,
domExceptionStackSetter, error))) {
On 2016/08/09 07:53:50, haraken wrote:
> 
> Is it possible that the SetAccessor() call fails?

Unlikely.

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:109:
DEFINE_CREATE_AND_THROW_ERROR_FUNC(GeneralError, Error, "Error")
On 2016/08/09 07:53:50, haraken wrote:
> 
> Shall we rename GeneralError to Error in a follow-up CL? I guess GeneralError
is
> not a speced error.

Yes, will do.

haraken

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp File third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp (left): https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp#oldcode73 third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:73: if (!frame || !BindingSecurity::shouldAllowAccessToFrame(isolate, currentDOMWindow(isolate), frame, DoNotReportSecurityError)) { On ...

4 years, 4 months ago (2016-08-10 05:00:33 UTC) #17

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp (left):

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:73: if (!frame
|| !BindingSecurity::shouldAllowAccessToFrame(isolate,
currentDOMWindow(isolate), frame, DoNotReportSecurityError)) {
On 2016/08/10 03:11:10, Yuki wrote:
> On 2016/08/09 07:53:50, haraken wrote:
> > 
> > Do you know why we had this security check? This CL makes sense but I want
to
> > make sure that we're not removing code that shouldn't be removed.
> 
> The old implementation was creating a new exception in the given context
> (|creationContext|) so we need to check that the current realm has the right
> access to that context.
> 
> Now, we always create an exception in the current realm, there is no need to
> check.

Makes sense.

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp (right):

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:82: if
(tryCatch.HasCaught()) {
On 2016/08/10 03:11:10, Yuki wrote:
> On 2016/08/09 07:53:50, haraken wrote:
> > 
> > Just help me understand: Who can throw an exception?
> 
> OOM (out of handles or stack) in V8?
> 
> Probably the original author wanted to make it sure that this function
*creates*
> an exception, does not *throw*.
> 
> I didn't realize this unlikeliness.  May I remove this TryCatch block?

Yeah, let's remove it.

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:87: if
(!v8CallBoolean(exceptionObj->SetAccessor(isolate->GetCurrentContext(),
v8AtomicString(isolate, "stack"), domExceptionStackGetter,
domExceptionStackSetter, error))) {
On 2016/08/10 03:11:10, Yuki wrote:
> On 2016/08/09 07:53:50, haraken wrote:
> > 
> > Is it possible that the SetAccessor() call fails?
> 
> Unlikely.

Then let's use ToChecked.

Yuki

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

4 years, 4 months ago (2016-08-10 05:44:32 UTC) #18

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-10 05:44:53 UTC) #19

Yuki

https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp File third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp (right): https://codereview.chromium.org/2209203002/diff/80001/third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp#newcode82 third_party/WebKit/Source/bindings/core/v8/V8ThrowException.cpp:82: if (tryCatch.HasCaught()) { On 2016/08/10 05:00:33, haraken wrote: > ...

4 years, 4 months ago (2016-08-10 05:45:11 UTC) #20

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-10 10:01:48 UTC) #22

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-08-10 10:01:49 UTC) #23

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-10 10:02:55 UTC) #25

commit-bot: I haz the power

Description was changed from ========== binding: Always throws an exception in the current realm. Rewrote ...

4 years, 4 months ago (2016-08-10 10:09:10 UTC) #27

commit-bot: I haz the power

4 years, 4 months ago (2016-08-10 10:09:11 UTC) #28

Message was sent while issue was closed.

Patchset 6 (id:??) landed as
https://crrev.com/d375d0327f654294eeb1e0d7bcae0829dcd89ae4
Cr-Commit-Position: refs/heads/master@{#411016}

Issue 2209203002: binding: Always throws an exception in the current realm. (Closed)

Description

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Patch Set 4 : Synced. #

Patch Set 5 : Updated test expectations. #

Patch Set 6 : Addressed review comments. #

Messages