Bug 98108 – Correct detection of context type in WorldContextHandle

• Component: New Bugs
• Status: UNCONFIRMED

[Dan Carney]

Correct detection of context type in WorldContextHandle

[Dan Carney]

Created attachment 166596 [details]
Patch

[Kentaro Hara]

Comment on attachment 166596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166596&action=review

Abarth: review?

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:190
> +    if (!wrapper->IsNumber() && !wrapper->IsExternal())

!wrapper->IsNumber() || !wrapper->IsExternal() ?

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:50
> +    // FIXME: This probably shouldn't be allowed to happen.
>      if (v8::Context::InContext()) {

In what situation do you think this can happen?

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:60
> +    if (UNLIKELY(typeInfo == &V8DedicatedWorkerContext::info)) {

UNLIKELY() would not be helpful in .cpp. You can remove it.

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:65
> +    if (UNLIKELY(typeInfo == &V8SharedWorkerContext::info)) {

Ditto.

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:73
> +    if (UNLIKELY(typeInfo != &V8DOMWindow::info)) {

Ditto.

[Adam Barth]

Comment on attachment 166596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166596&action=review

>> Source/WebCore/bindings/v8/WorldContextHandle.cpp:60
>> +    if (UNLIKELY(typeInfo == &V8DedicatedWorkerContext::info)) {
> 
> UNLIKELY() would not be helpful in .cpp. You can remove it.

Actually, UNLIKELY and LIKELY don't do anything on the compilers and architectures we use.  We might want to just remove them from the project.

[Adam Barth]

Regressions: Unexpected crashes : (24)

[Adam Barth]

Comment on attachment 166596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166596&action=review

> Source/WebCore/ChangeLog:9
> +        It's possible that certain contexts get incorrectly detected as worker
> +        contexts.

Can you write a test that demonstrates this issue?

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:180
> +WrapperTypeInfo* V8DOMWrapper::domWrapperTypeSlow(v8::Handle<v8::Value> value)

Adding this function seems like the wrong approach.  We shouldn't be calling this function with a random object.  We're starting from a v8::Context.  We should know how to find the right JS object to call domWrapperType on.

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:52
> +        m_worldToUse = UseMainWorld;
> +        return;

This seems to say that we'll always use the UseMainWorld if we call this function while JavaScript is on the stack.

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:55
> +    v8::Handle<v8::Context> context = v8::Context::GetCurrent();

If we've reached this line, v8::Context::InContext() must have returned false, which means there is no current context.

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 166596 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166596&action=review
> 
> > UNLIKELY() would not be helpful in .cpp. You can remove it.
> 
> Actually, UNLIKELY and LIKELY don't do anything on the compilers and architectures we use.  We might want to just remove them from the project.

(As far as I remember) I've observed that the generated assembly changes in clang and it affects performance (e.g. https://bugs.webkit.org/attachment.cgi?id=166147&action=review). A branch marked as UNLIKELY() is likely to be placed after the other branch (to place hot call paths as sequentially as possible).

Either way, LIKELY/UNLIKELY in outline functions would have no meaning.

[Adam Barth]

> I've observed that the generated assembly changes in clang

They compile to nothing in clang:

http://trac.webkit.org/browser/trunk/Source/WTF/wtf/Compiler.h#L188

[Dan Carney]

Comment on attachment 166596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=166596&action=review

>> Source/WebCore/ChangeLog:9
>> +        contexts.
> 
> Can you write a test that demonstrates this issue?

I'm not sure there's a test that can hit this in a predictable manner, but the issue came up because of http://code.google.com/p/chromium/issues/detail?id=150737

>> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:180
>> +WrapperTypeInfo* V8DOMWrapper::domWrapperTypeSlow(v8::Handle<v8::Value> value)
> 
> Adding this function seems like the wrong approach.  We shouldn't be calling this function with a random object.  We're starting from a v8::Context.  We should know how to find the right JS object to call domWrapperType on.

Fair enough. For the case this patch is trying to fix, we want to know if it's V8SharedWorkerContext, V8DedicatedWorkerContext or V8DOMWindow or something else, because the context is some unknown context and I didn't want to call isWrapperOfType multiple times.  But probably, yes, the fix for that particular issue is to use the correct context for IndexedDB deserialization.  I also wanted to use this function in https://bugs.webkit.org/show_bug.cgi?id=96637, where the context is totally unknown, and I've hit assertions using isWrapperOfType.  I was hoping to replace isWrapperOfType with this function.

>> Source/WebCore/bindings/v8/WorldContextHandle.cpp:52
>> +        return;
> 
> This seems to say that we'll always use the UseMainWorld if we call this function while JavaScript is on the stack.

Yeah, that should be !v8::Context::InContext() - I'm on vacation on with a very slow editing environment.  Returning UseMainWorld is the current (albeit unusual) behaviour.

>>> Source/WebCore/bindings/v8/WorldContextHandle.cpp:60
>>> +    if (UNLIKELY(typeInfo == &V8DedicatedWorkerContext::info)) {
>> 
>> UNLIKELY() would not be helpful in .cpp. You can remove it.
> 
> Actually, UNLIKELY and LIKELY don't do anything on the compilers and architectures we use.  We might want to just remove them from the project.

okay

[Dan Carney]

Created attachment 166888 [details]
Patch

[Build Bot]

Comment on attachment 166888 [details]
Patch

Attachment 166888 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14124871

New failing tests:
http/tests/workers/terminate-during-sync-operation.html