Bug 96637 – Remove V8DOMWindowShell::getEntered

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dan Carney]

Remove V8DOMWindowShell::getEntered

[Dan Carney]

Created attachment 163847 [details]
Patch

[Adam Barth]

Comment on attachment 163847 [details]
Patch

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

This looks great!  A few very minor comments below.

I suspect we'll eventually to unify this data with V8PerContextData, but currently the lifetimes are a bit different.  This is great for now though.

> Source/WebCore/bindings/v8/ScopedPersistent.h:86
> +    void clearWeak(void* parameter, v8::WeakReferenceCallback callback)
> +    {
> +        ASSERT(!m_handle.IsEmpty());
> +        m_handle.MakeWeak(parameter, callback);
> +        m_handle.Clear();
> +    }

Rather than encouraging this tricky lifetime semantics by adding this feature to ScopedPersistent, it might be better if we provide a way to leak the handle.  For example:

v8::Persistent<T> leakHandle()
{
    v8::Persistent<T> handle = m_handle;
    m_handel.Clear();
    return handle;
}

Then the caller can write:

m_whatever.leakHandle().MakeWeak(...)

and then the caller will know that it is responsible for disposing the handle because the word "leak" appears at the call site.

> Source/WebCore/bindings/v8/ScriptController.cpp:398
> +            delete isolatedWorldShell;

Normally I would complain about raw calls to delete, but I know you're going to make these into OwnPtrs shortly.  :)

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:179
> -V8DOMWindowShell* V8DOMWindowShell::enteredIsolatedWorldContext()
> +V8DOMWindowShell::IsolatedContextData* V8DOMWindowShell::enteredIsolatedContextData(v8::Local<v8::Context> context)

enteredIsolatedContextData -> toIsolatedContextData

The word "entered" in these functions refers to the V8 API concept of the "entered" v8::Context.  For example, in v8::Context::GetEntered().  Here, you're asking the caller to supply the context.  They might supply the entered context or they might supply whatever context they want.

nit: Local -> Handle.  We try to use the more general type whenever it doesn't mater.  I see that you're mirroring setIsolatedWorldField, but that should also use Handle rather than Local.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:360
> +        m_isolatedContextData = IsolatedContextData::create(m_world.get(), m_perContextData.release(), m_isolatedWorldShellSecurityOrigin.get());

Can't you skip these get() calls?

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:622
> +        m_isolatedContextData->setSecurityOrigin(m_isolatedWorldShellSecurityOrigin.get());

ditto

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:71
> +    private:

This line should have a blank line before it.

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:73
> +        IsolatedContextData(PassRefPtr<DOMWrapperWorld> world, PassOwnPtr<V8PerContextData> perContextData,
> +                PassRefPtr<SecurityOrigin> securityOrigin)

This can all go on one line.

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:76
> +            , m_securityOrigin(securityOrigin) { }

These { } should each be on a separate line and have a blank line after them.

[Dan Carney]

Created attachment 164063 [details]
Patch

[Adam Barth]

Comment on attachment 164063 [details]
Patch

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

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:77
> +        { }

style nit: technically they each get their own line.  Sorry for being unclear before.

[Adam Barth]

Don't worry about the style nit.  We can clean it up in a later patch to this file.

[Dan Carney]

(In reply to comment #4)
> (From update of attachment 164063 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164063&action=review
> 
> > Source/WebCore/bindings/v8/V8DOMWindowShell.h:77
> > +        { }
> 
> style nit: technically they each get their own line.  Sorry for being unclear before.

Oh. Gotcha. Yeah, next time.  I was planning for that class to be short lived anyway.

[WebKit Review Bot]

Comment on attachment 164063 [details]
Patch

Clearing flags on attachment: 164063

Committed r128566: <http://trac.webkit.org/changeset/128566>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

The patch seems to cause an assertion failure in v8. I'll roll it out.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=close-code-and-reason.html

crash log for DumpRenderTree (pid 2387):
STDOUT: 
STDOUT: ==== Stack trace ============================================
STDOUT: 
STDOUT: Security context: 0x2e034d75 <JS Object>#0#
STDOUT:     1: /* anonymous */ [http://127.0.0.1:8000/websocket/tests/hybi/workers/resources/close-code-and-reason.js:18] (this=0x2e034e1d <JS Global Object>#1#)
STDOUT: 
STDOUT: ==== Details ================================================
STDOUT: 
STDOUT: [1]: /* anonymous */ [http://127.0.0.1:8000/websocket/tests/hybi/workers/resources/close-code-and-reason.js:18] (this=0x2e034e1d <JS Global Object>#1#) {
STDOUT:   // stack-allocated locals
STDOUT:   var .result = 0x2e008091 <undefined>
STDOUT:   // expression stack (top to bottom)
STDOUT:   [03] : 0x2e037535 <JS Function>#2#
STDOUT:   [02] : 0x2aa15409 <String[6]: onopen>
STDOUT:   [01] : 0x4770d9f9 <a WebSocket>#3#
STDOUT: --------- s o u r c e   c o d e ---------
STDOUT: var codeNormalClosure = 1000;?var codeNoStatusRcvd = 1005;?var codeAbnormalClosure = 1006;?var emptyString = "";??function postResult(result, actual, expected)?{?    var message = result ? "PASS" : "FAIL";?    message += ": worker: " + actual + " is ";?    if (!result)?        message += "not ";?   ...
STDOUT: 
STDOUT: -----------------------------------------
STDOUT: }
STDOUT: 
STDOUT: ==== Key         ============================================
STDOUT: 
STDOUT:  #0# 0x2e034d75: 0x2e034d75 <JS Object>
STDOUT:  #1# 0x2e034e1d: 0x2e034e1d <JS Global Object>
STDOUT:  #2# 0x2e037535: 0x2e037535 <JS Function>
STDOUT:  #3# 0x4770d9f9: 0x4770d9f9 <a WebSocket>
STDOUT: =====================
STDOUT: 
STDERR: 
STDERR: 
STDERR: #
STDERR: # Fatal error in ../../src/objects-inl.h, line 1487
STDERR: # CHECK(index < GetInternalFieldCount() && index >= 0) failed
STDERR: #
STDERR: 
STDERR: [2387:-1602484928:1847752401410:ERROR:process_util_posix.cc(143)] Received signal 6

[WebKit Review Bot]

Re-opened since this is blocked by 96760

[Dan Carney]

(In reply to comment #9)
> The patch seems to cause an assertion failure in v8. I'll roll it out.
> 


I don't think it's this patch causing the flake.  It just happens to be hitting code I modified.  What it's trying to do is completely unreasonable, both before and after this patch.

[Kent Tamura]

(In reply to comment #11)
> (In reply to comment #9)
> > The patch seems to cause an assertion failure in v8. I'll roll it out.
> > 
> 
> 
> I don't think it's this patch causing the flake.  It just happens to be hitting code I modified.  What it's trying to do is completely unreasonable, both before and after this patch.

> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&showLargeExpectations=true&tests=close-code-and-reason.html

It's clear that CRASH failures started since r128566, and stopped by the rollout.

[Dan Carney]

Created attachment 164227 [details]
Patch

rebased after https://bugs.webkit.org/show_bug.cgi?id=96790

[WebKit Review Bot]

Comment on attachment 164227 [details]
Patch

Clearing flags on attachment: 164227

Committed r128669: <http://trac.webkit.org/changeset/128669>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

(In reply to comment #14)
> (From update of attachment 164227 [details])
> Clearing flags on attachment: 164227
> 
> Committed r128669: <http://trac.webkit.org/changeset/128669>

I'm afraid it broke another test.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=text-encoding.html

I'll roll it out again...

[Kent Tamura]

Reverted r128669 for reason:

Broke http/tests/workers/text-encoding.html on Chromium Linux (dbg)

Committed r128687: <http://trac.webkit.org/changeset/128687>

[Kent Tamura]

(In reply to comment #17)
> Reverted r128669 for reason:
> 
> Broke http/tests/workers/text-encoding.html on Chromium Linux (dbg)
> 
> Committed r128687: <http://trac.webkit.org/changeset/128687>

Confirmed the rollout fixed the crash.

[Dan Carney]

Created attachment 164358 [details]
Patch

[Dan Carney]

(In reply to comment #19)
> Created an attachment (id=164358) [details]
> Patch

handling ScriptController::currentWorldContext differently this iteration

[Dan Carney]

Created attachment 164371 [details]
Patch

[Adam Barth]

> handling ScriptController::currentWorldContext differently this iteration

Given that we've had some trouble landing this patch, how would you feel about waiting until Wednesday to land it?  That should be after the M23 branch and will give us more time to catch problems in the Dev channel.

[Adam Barth]

Comment on attachment 164371 [details]
Patch

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

> Source/WebCore/bindings/v8/ScriptController.cpp:447
>  v8::Local<v8::Context> ScriptController::currentWorldContext()
>  {
> -    if (V8DOMWindowShell* isolatedShell = V8DOMWindowShell::getEntered()) {
> -        v8::Persistent<v8::Context> context = isolatedShell->context();
> -        if (context.IsEmpty() || m_frame != toFrameIfNotDetached(context))
> +    V8DOMWindowShell::IsolatedContextData* isolatedContextData = V8DOMWindowShell::enteredIsolatedContextData();
> +    if (UNLIKELY(!!isolatedContextData)) {
> +        V8DOMWindowShell* isolatedShell = existingWindowShellInternal(isolatedContextData->world());
> +        // A temporary isolated world has been deleted, so use the current context.
> +        if (UNLIKELY(!isolatedShell)) {
> +            v8::Handle<v8::Context> context = v8::Context::GetEntered();
> +            if (m_frame != toFrameIfNotDetached(context))
> +                return v8::Local<v8::Context>();
> +            return v8::Local<v8::Context>::New(context);
> +        }
> +        // The shell exists, but potentially it has a new context, so use it.
> +        if (isolatedShell->context().IsEmpty() || m_frame != toFrameIfNotDetached(isolatedShell->context()))
>              return v8::Local<v8::Context>();
> -        return v8::Local<v8::Context>::New(context);
> +        return v8::Local<v8::Context>::New(isolatedShell->context());
>      }

This function is getting way too complicated.  What's causing this complexity?  Perhaps we need to remove the concept of a temporary isolated world?  Maybe we need to treat temporary isolated worlds more like regular isolated worlds?

[Dan Carney]

(In reply to comment #22)
> > handling ScriptController::currentWorldContext differently this iteration
> 
> Given that we've had some trouble landing this patch, how would you feel about waiting until Wednesday to land it?  That should be after the M23 branch and will give us more time to catch problems in the Dev channel.

Sounds good.

[Dan Carney]

(In reply to comment #23)
> (From update of attachment 164371 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=164371&action=review
> 
> > Source/WebCore/bindings/v8/ScriptController.cpp:447
> >  v8::Local<v8::Context> ScriptController::currentWorldContext()
> >  {
> > -    if (V8DOMWindowShell* isolatedShell = V8DOMWindowShell::getEntered()) {
> > -        v8::Persistent<v8::Context> context = isolatedShell->context();
> > -        if (context.IsEmpty() || m_frame != toFrameIfNotDetached(context))
> > +    V8DOMWindowShell::IsolatedContextData* isolatedContextData = V8DOMWindowShell::enteredIsolatedContextData();
> > +    if (UNLIKELY(!!isolatedContextData)) {
> > +        V8DOMWindowShell* isolatedShell = existingWindowShellInternal(isolatedContextData->world());
> > +        // A temporary isolated world has been deleted, so use the current context.
> > +        if (UNLIKELY(!isolatedShell)) {
> > +            v8::Handle<v8::Context> context = v8::Context::GetEntered();
> > +            if (m_frame != toFrameIfNotDetached(context))
> > +                return v8::Local<v8::Context>();
> > +            return v8::Local<v8::Context>::New(context);
> > +        }
> > +        // The shell exists, but potentially it has a new context, so use it.
> > +        if (isolatedShell->context().IsEmpty() || m_frame != toFrameIfNotDetached(isolatedShell->context()))
> >              return v8::Local<v8::Context>();
> > -        return v8::Local<v8::Context>::New(context);
> > +        return v8::Local<v8::Context>::New(isolatedShell->context());
> >      }
> 
> This function is getting way too complicated.  What's causing this complexity?  Perhaps we need to remove the concept of a temporary isolated world?  Maybe we need to treat temporary isolated worlds more like regular isolated worlds?

I think ultimately this function needs some refactoring.  It's odd to want to retrieve the entered context and then not use it, but instead use the context from the same world that it currently attached to the frame.  I suspect there are two distinct use cases for this function, and we are using it for both.  I also don't fully understand why the main world case gets initialized if it's empty and the isolated world case doesn't.

Anyway, that function is slated for a major reworking anyway to fix bug 93646, so I'd like to hold off until then if this patch stops hitting assertions in worker threads.

[Dan Carney]

Comment on attachment 164371 [details]
Patch

dropping review flag until after milestone  branch

[Dan Carney]

Created attachment 166267 [details]
Patch

[Adam Barth]

Comment on attachment 166267 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Binding.cpp:314
> +    V8DOMWindowShell::IsolatedContextData* isolatedShellData;

Would you be willing to initialize this scalar to 0? It always makes me worry to see uninitialized pointers.

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:63
> +        static PassOwnPtr<IsolatedContextData> create(PassRefPtr<DOMWrapperWorld> world, PassOwnPtr<V8PerContextData> perContextData,
> +            PassRefPtr<SecurityOrigin> securityOrigin)

You can merge these lines.

[Adam Barth]

Interesting.  This patch is telling us that we should make the lifetime of V8PerContextData match that of v8::Context.  Currently, V8PerContextData is only alive for main world contexts when those contexts are displayed in a frame.  You're essentially extending the lifetime of V8PerContextData in the case of isolated worlds to match that of the underlying v8::Context.

We'll eventually want to further align the implementation of the main and non-main world, but I think this patch is a good step.

[Adam Barth]

Feel free to set the commit-queue flag to ? if you'd like me (or someone else) to mark it commit-queue+.

[Dan Carney]

Comment on attachment 166267 [details]
Patch

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

>> Source/WebCore/bindings/v8/V8Binding.cpp:314
>> +    V8DOMWindowShell::IsolatedContextData* isolatedShellData;
> 
> Would you be willing to initialize this scalar to 0? It always makes me worry to see uninitialized pointers.

done

>> Source/WebCore/bindings/v8/V8DOMWindowShell.h:63
>> +            PassRefPtr<SecurityOrigin> securityOrigin)
> 
> You can merge these lines.

done

[Dan Carney]

Created attachment 166330 [details]
Patch

[WebKit Review Bot]

Comment on attachment 166330 [details]
Patch

Clearing flags on attachment: 166330

Committed r129965: <http://trac.webkit.org/changeset/129965>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ojan Vafai]

This patch hits an assert running http/tests/workers/text-encoding.html on Chromium Linux debug.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=http%2Ftests%2Fworkers%2Ftext-encoding.html

STDERR: ASSERTION FAILED: innerGlobal->InternalFieldCount() >= V8DOMWindow::enteredIsolatedWorldIndex
STDERR: third_party/WebKit/Source/WebCore/bindings/v8/V8DOMWindowShell.cpp(180) : static WebCore::V8DOMWindowShell::IsolatedContextData* WebCore::V8DOMWindowShell::toIsolatedContextData(v8::Handle<v8::Object>)
STDERR: [22109:22892:16409146392669:ERROR:process_util_posix.cc(144)] Received signal 11
STDERR: 	base::debug::StackTrace::StackTrace() [0x7f555a75fc6a]
STDERR: 	base::(anonymous namespace)::StackDumpSignalHandler() [0x7f555a7c8d59]
STDERR: 	0x7f5553a14af0
STDERR: 	WebCore::V8DOMWindowShell::toIsolatedContextData() [0x7f555dca621d]
STDERR: 	WebCore::V8DOMWindowShell::enteredIsolatedContextData() [0x7f555ce69485]
STDERR: 	WebCore::V8XMLHttpRequest::constructorCallback() [0x7f555dcfb016]
STDERR: 	v8::internal::HandleApiCallHelper<>() [0x7f555abdfbc8]
STDERR: 	v8::internal::Builtin_Impl_HandleApiCallConstruct() [0x7f555abda2df]
STDERR: 	v8::internal::Builtin_HandleApiCallConstruct() [0x7f555abda2b0]
STDERR: 	0x253e4e80618e

[Ojan Vafai]

Also causes fast/files/workers/worker-apply-blob-url-to-xhr.html to sometimes hit an assert.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Ffiles%2Fworkers%2Fworker-apply-blob-url-to-xhr.html

STDERR: ASSERTION FAILED: innerGlobal->InternalFieldCount() >= V8DOMWindow::enteredIsolatedWorldIndex
STDERR: /Volumes/data/b/build/slave/webkit-mac-latest-dbg/build/src/third_party/WebKit/Source/WebCore/WebCore.gyp/../bindings/v8/V8DOMWindowShell.cpp(180) : static V8DOMWindowShell::IsolatedContextData *WebCore::V8DOMWindowShell::toIsolatedContextData(v8::Handle<v8::Object>)
STDERR: 1   0x5751c3a7 WebCore::V8DOMWindowShell::toIsolatedContextData(v8::Handle<v8::Object>)
STDERR: 2   0x574c6094 WebCore::V8DOMWindowShell::enteredIsolatedContextData()
STDERR: 3   0x575b658a WebCore::V8XMLHttpRequest::constructorCallback(v8::Arguments const&)
STDERR: 4   0x5d340f92 v8::internal::MaybeObject* v8::internal::HandleApiCallHelper<true>(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*)
STDERR: 5   0x5d34091a v8::internal::Builtin_Impl_HandleApiCallConstruct(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*)
STDERR: 6   0x5d3374ac v8::internal::Builtin_HandleApiCallConstruct(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*)
STDERR: 7   0x51f0a336
STDERR: [3067:-1298067456:2487180718778:ERROR:process_util_posix.cc(144)] Received signal 11
STDERR: 	0   libbase.dylib                       0x5e344d8f base::debug::StackTrace::StackTrace() + 63
STDERR: 	1   libbase.dylib                       0x5e344d2b base::debug::StackTrace::StackTrace() + 43
STDERR: 	2   libbase.dylib                       0x5e41e4f7 base::(anonymous namespace)::StackDumpSignalHandler(int, __siginfo*, __darwin_ucontext*) + 295
STDERR: 	3   libSystem.B.dylib                   0x9539405b _sigtramp + 43
STDERR: 	4   ???                                 0xffffffff 0x0 + 4294967295
STDERR: 	5   libwebkit.dylib                     0x574c6094 WebCore::V8DOMWindowShell::enteredIsolatedContextData() + 180
STDERR: 	6   libwebkit.dylib                     0x575b658a WebCore::V8XMLHttpRequest::constructorCallback(v8::Arguments const&) + 186
STDERR: 	7   libv8.dylib                         0x5d340f92 v8::internal::MaybeObject* v8::internal::HandleApiCallHelper<true>(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) + 1602
STDERR: 	8   libv8.dylib                         0x5d34091a v8::internal::Builtin_Impl_HandleApiCallConstruct(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) + 74
STDERR: 	9   libv8.dylib                         0x5d3374ac v8::internal::Builtin_HandleApiCallConstruct(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) + 172
STDERR: 	10  ???                                 0x51f0a336 0x0 + 1374724918
STDERR: ax: bbadbeef, bx: 5d337400, cx: 5b30e34c, dx: 5b30e34c
STDERR: di: 58842c8e, si: 58842c17, bp: b2a10108, sp: b2a100d0, ss: 23, flags: 10282
STDERR: ip: 5751c3b1, cs: 1b, ds: 23, es: 23, fs: 23, gs: f

[Adam Barth]

Sounds like we should roll it out.  It's not quite working right for workers.

[WebKit Review Bot]

Re-opened since this is blocked by bug 97970

[Dan Carney]

Created attachment 166385 [details]
Patch

[Dan Carney]

Comment on attachment 166385 [details]
Patch

replaced failing assert with if clause

[Adam Barth]

Comment on attachment 166385 [details]
Patch

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

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:180
> +    if (innerGlobal->InternalFieldCount() < V8DOMWindow::enteredIsolatedWorldIndex)

This is really fragile. There's no guarantee that the innerGlobal for a WorkerContext will have fewer internal fields than the innerGlobal for a DOMWindow.

It looks like this function used to only be called on the main thread. We probably need to avoid calling it on worker threads.

[Dan Carney]

(In reply to comment #41)
> (From update of attachment 166385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=166385&action=review
> 
> > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:180
> > +    if (innerGlobal->InternalFieldCount() < V8DOMWindow::enteredIsolatedWorldIndex)
> 
> This is really fragile. There's no guarantee that the innerGlobal for a WorkerContext will have fewer internal fields than the innerGlobal for a DOMWindow.
> 
> It looks like this function used to only be called on the main thread. We probably need to avoid calling it on worker threads.

I'm not positive, but I believe it has always been used on both worker and main threads.  In the stack trace above, for instance, it is can called from a worked thread or a main thread.

I can change the if clause to check if we're on the main thread?

[Adam Barth]

Comment on attachment 166385 [details]
Patch

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

>>> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:180
>>> +    if (innerGlobal->InternalFieldCount() < V8DOMWindow::enteredIsolatedWorldIndex)
>> 
>> This is really fragile. There's no guarantee that the innerGlobal for a WorkerContext will have fewer internal fields than the innerGlobal for a DOMWindow.
>> 
>> It looks like this function used to only be called on the main thread. We probably need to avoid calling it on worker threads.
> 
> I'm not positive, but I believe it has always been used on both worker and main threads.  In the stack trace above, for instance, it is can called from a worked thread or a main thread.
> 
> I can change the if clause to check if we're on the main thread?

The static_cast<V8DOMWindowShell*> would have been problematic on a worker thread because V8DOMWindowShell* is a main-thread only class.  Maybe we introduced this bug in a previous patch and didn't notice?

> Source/WebCore/bindings/v8/custom/V8XMLHttpRequestConstructor.cpp:-59
> -    if (V8DOMWindowShell* isolatedWorldShell = V8DOMWindowShell::getEntered())
> -        securityOrigin = isolatedWorldShell->isolatedWorldSecurityOrigin();

Yes, this one looks problematic.  This function can be called on a worker thread.  You can add a check here for context->isDocument().  There's no need to look for isolatedWorldSecurityOrigin if we're not on the main thread (i.e., if context->isDocument() is false).

[Dan Carney]

> 
> The static_cast<V8DOMWindowShell*> would have been problematic on a worker thread because V8DOMWindowShell* is a main-thread only class.  Maybe we introduced this bug in a previous patch and didn't notice?

It's certainly possible, and I'm wondering if it's related to http://code.google.com/p/chromium/issues/detail?id=150737#c36

I feel like I need to replicate the logic in WorldContextHandle:: WorldContextHandle but I'm not convinced at the moment that this is even the correct logic.  Do you know if there is a better way to check if a context is a worker thread?

[Adam Barth]

Comment on attachment 166385 [details]
Patch

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

> Source/WebCore/bindings/v8/WorldContextHandle.cpp:58
> +        return;

In the worker case, it looks like we'll return early here.

[Dan Carney]

Created attachment 175227 [details]
Patch

[Dan Carney]

(In reply to comment #46)
> Created an attachment (id=175227) [details]
> Patch

This patch got a lot simpler due to recent refactoring.

[Adam Barth]

Comment on attachment 175227 [details]
Patch

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

Very nice.

> Source/WebCore/bindings/v8/DOMWrapperWorld.h:58
> +

By the way, is there a reason we have:

class DOMWrapperWorld : public WTF::RefCountedBase

The normal pattern is

class DOMWrapperWorld : public RefCounted<DOMWrapperWorld>

Sorry if I asked you that before and forgot.

[WebKit Review Bot]

Comment on attachment 175227 [details]
Patch

Clearing flags on attachment: 175227

Committed r135293: <http://trac.webkit.org/changeset/135293>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dan Carney]

(In reply to comment #48)
> (From update of attachment 175227 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175227&action=review
> 
> Very nice.
> 
> > Source/WebCore/bindings/v8/DOMWrapperWorld.h:58
> > +
> 
> By the way, is there a reason we have:
> 
> class DOMWrapperWorld : public WTF::RefCountedBase
> 
> The normal pattern is
> 
> class DOMWrapperWorld : public RefCounted<DOMWrapperWorld>
> 
> Sorry if I asked you that before and forgot.


I think there was an edge case when I started all this refactored where the main world could be deleted or something, and this was the only way to intercept the destructor. I should have added a FIXME when I added it. Anyway, it can be removed now. I'll submit a fix tomorrow.

[WebKit Review Bot]

Re-opened since this is blocked by bug 102832

[Jian Li]

This seems to cause crash to some layout tests:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=http%2Ftests%2Fsecurity%2FisolatedWorld%2Fclick-event.html%2Chttp%2Ftests%2Fsecurity%2FisolatedWorld%2Fworld-reuse.html%2Chttp%2Ftests%2Fsecurity%2FisolatedWorld%2Fglobal-variables.html%2Chttp%2Ftests%2Fsecurity%2FisolatedWorld%2Fdocument-open.html%2Chttp%2Ftests%2Fsecurity%2FisolatedWorld%2FdidClearWindowObject.html%2Chttp%2Ftests%2Fsecurity%2FisolatedWorld%2Fwindow-properties.html

crash log for DumpRenderTree (pid 3180):
STDOUT: V8 error: Cannot create a handle without a HandleScope (v8::HandleScope::CreateHandle()).  Current memory usage: 71 MB
STDERR: Backtrace:
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x61D4ED67+17012663]
STDERR: 	v8::Testing::operator= [0x6C804811+3761]
STDERR: 	v8::Locker::StopPreemption [0x6C898FCE+376158]
STDERR: 	v8::Testing::DeoptimizeAll [0x6C83AC18+34408]
STDERR: 	v8::Testing::DeoptimizeAll [0x6C835DC0+14352]
STDERR: 	v8::Context::SlowGetEmbedderData [0x6C80717F+1135]
STDERR: 	v8::Context::SlowGetAlignedPointerFromEmbedderData [0x6C807E1B+59]
STDERR: 	v8::Isolate::Scope::Scope [0x6C80355A+8832]
STDERR: 	WebKit::WebFilterOperation::WebFilterOperation [0x617871E8+10951736]

[Dan Carney]

Comment on attachment 175227 [details]
Patch

rollback error may not have been caused by this patch

Comment on attachment 175227 [details]
Patch

I couldn't reproduce the failure myself, and some other patch was also rolled back with the same backtrace as reason. Let's just give it another try

[WebKit Review Bot]

Comment on attachment 175227 [details]
Patch

Rejecting attachment 175227 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
pp
Hunk #1 succeeded at 94 (offset -98 lines).
Hunk #2 succeeded at 106 (offset -98 lines).
Hunk #3 succeeded at 211 (offset -98 lines).
Hunk #4 succeeded at 221 (offset -98 lines).
patching file Source/WebCore/bindings/v8/V8DOMWindowShell.h
patching file Source/WebCore/bindings/v8/V8PerContextData.h
patching file Source/WebCore/bindings/v8/WorldContextHandle.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14898038

[Adam Barth]

Created attachment 175552 [details]
Patch for landing

[Adam Barth]

I have attempted to rebase this patch to HEAD.

[WebKit Review Bot]

Comment on attachment 175552 [details]
Patch for landing

Rejecting attachment 175552 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/14968169

[WebKit Review Bot]

Comment on attachment 175552 [details]
Patch for landing

Clearing flags on attachment: 175552

Committed r135479: <http://trac.webkit.org/changeset/135479>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dan Carney]

Reopening to attach new patch.

[Dan Carney]

Created attachment 175757 [details]
Patch

[Dan Carney]

Comment on attachment 175757 [details]
Patch

windows 7 debug issue probably fixed

[WebKit Review Bot]

Comment on attachment 175757 [details]
Patch

Clearing flags on attachment: 175757

Committed r135601: <http://trac.webkit.org/changeset/135601>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.