Bug 96637

Created attachment 163847 [details] Patch

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.

Created attachment 164063 [details] Patch

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.

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

(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.

Comment on attachment 164063 [details] Patch Clearing flags on attachment: 164063 Committed r128566: <http://trac.webkit.org/changeset/128566>

All reviewed patches have been landed. Closing bug.

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

Re-opened since this is blocked by 96760

(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.

(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.

Created attachment 164227 [details] Patch rebased after https://bugs.webkit.org/show_bug.cgi?id=96790

Comment on attachment 164227 [details] Patch Clearing flags on attachment: 164227 Committed r128669: <http://trac.webkit.org/changeset/128669>

All reviewed patches have been landed. Closing bug.

(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...

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

(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.

Created attachment 164358 [details] Patch

(In reply to comment #19) > Created an attachment (id=164358) [details] > Patch handling ScriptController::currentWorldContext differently this iteration

Created attachment 164371 [details] Patch

> 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.

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?

(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.

(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.

Comment on attachment 164371 [details] Patch dropping review flag until after milestone branch

Created attachment 166267 [details] Patch

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.

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.

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

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

Created attachment 166330 [details] Patch

Comment on attachment 166330 [details] Patch Clearing flags on attachment: 166330 Committed r129965: <http://trac.webkit.org/changeset/129965>

All reviewed patches have been landed. Closing bug.

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

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

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

Re-opened since this is blocked by bug 97970

Created attachment 166385 [details] Patch

Comment on attachment 166385 [details] Patch replaced failing assert with if clause

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.

(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?

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).

> > 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?

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.

Created attachment 175227 [details] Patch

(In reply to comment #46) > Created an attachment (id=175227) [details] > Patch This patch got a lot simpler due to recent refactoring.

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.

Comment on attachment 175227 [details] Patch Clearing flags on attachment: 175227 Committed r135293: <http://trac.webkit.org/changeset/135293>

All reviewed patches have been landed. Closing bug.

(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.

Re-opened since this is blocked by bug 102832

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]

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

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

Created attachment 175552 [details] Patch for landing

I have attempted to rebase this patch to HEAD.

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

Comment on attachment 175552 [details] Patch for landing Clearing flags on attachment: 175552 Committed r135479: <http://trac.webkit.org/changeset/135479>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

Created attachment 175757 [details] Patch

Comment on attachment 175757 [details] Patch windows 7 debug issue probably fixed

Comment on attachment 175757 [details] Patch Clearing flags on attachment: 175757 Committed r135601: <http://trac.webkit.org/changeset/135601>

All reviewed patches have been landed. Closing bug.