Bug 99975

Created attachment 169867 [details] Patch

Looks good to me, but abarth should look.

(In reply to comment #2) > Looks good to me, but abarth should look. Definitely. I have no idea why auxilliary contexts exist. They seem to me mostly like a good source of memory leaks, but I want some confirmation.

Comment on attachment 169867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169867&action=review > Source/WebCore/bindings/v8/V8Binding.cpp:313 > +v8::Local<v8::Context> toV8Context(ScriptExecutionContext* context) This function is too tempting for people to call, but it will often give the wrong result. The problem is that you don't know which worlds context you want to return, especially if there is not JavaScript on the stack. That's why the other version of this function requires a world context handle.

(In reply to comment #3) > Definitely. I have no idea why auxilliary contexts exist. They seem to me mostly like a good source of memory leaks, but I want some confirmation. Jumping in with history: The IndexedDB implementation used to have the Chromium "browser" process call via IPC into a "worker" process to do some IDB/V8 operations (given serialized script value and key path, return key; given SSV, key path and key, inject key and return new SSV). Note that this was stateless - SSV coming in, SSV or other value coming out immediately. This was creating a new V8 context on each call, which showed up as a CPU hotspot in profiling. We decided to cache the context and named it the "auxiliary context". To eliminate the "worker" process and the IPC overhead, these operations were moved to the "renderer" - either performed eagerly (as soon as script performed some IDB operation, before the task made it to the browser) or lazily (the browser would asynchronously notify a specific renderer to initiate a task where it would fetch values to operate on). The auxiliary context may be unnecessary at this point, but when the refactoring (above) was done it was retained based on discussions at the time. (alecflett@ can say more here)

(In reply to comment #5) > (In reply to comment #3) > > Definitely. I have no idea why auxilliary contexts exist. They seem to me mostly like a good source of memory leaks, but I want some confirmation. > > Jumping in with history: > > The IndexedDB implementation used to have the Chromium "browser" process call via IPC into a "worker" process to do some IDB/V8 operations (given serialized script value and key path, return key; given SSV, key path and key, inject key and return new SSV). Note that this was stateless - SSV coming in, SSV or other value coming out immediately. This was creating a new V8 context on each call, which showed up as a CPU hotspot in profiling. We decided to cache the context and named it the "auxiliary context". > > To eliminate the "worker" process and the IPC overhead, these operations were moved to the "renderer" - either performed eagerly (as soon as script performed some IDB operation, before the task made it to the browser) or lazily (the browser would asynchronously notify a specific renderer to initiate a task where it would fetch values to operate on). The auxiliary context may be unnecessary at this point, but when the refactoring (above) was done it was retained based on discussions at the time. (alecflett@ can say more here) Okay, thanks for the clarification. That makes sense. The problem is that generated code in v8 expects a certain prototype chain for the context it is executing in, and the current setup doesn't have that consistently. Sometimes code that should be executing in the context of a DOMWindow is being executed in the context of the default object (the auxilliaryContext) and vice versa. If the SSV transformations really are all stateless, and they all execute in the auxilliaryContext, it would probably be the best solution in terms of performance. I think though, that my patch will not cause a lot of performance overhead, and will execute in the way that the rest of the system works, which has some readability benefits and is less prone to incomprehensible v8 crashes.

Created attachment 170081 [details] Patch

(In reply to comment #4) > (From update of attachment 169867 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169867&action=review > > > Source/WebCore/bindings/v8/V8Binding.cpp:313 > > +v8::Local<v8::Context> toV8Context(ScriptExecutionContext* context) > > This function is too tempting for people to call, but it will often give the wrong result. The problem is that you don't know which worlds context you want to return, especially if there is not JavaScript on the stack. That's why the other version of this function requires a world context handle. Okay, I've modified toV8Context to take a WorldToUse parameter instead. It's much more efficient than the implicit conversion to WorldContextHandle that was happening before as it avoids a bunch of checks and the creation of a ScopedPersistent. I'd like to move all the toV8Context code eventually to WorldContextHandle from V8Binding, to make all this more clear.

Comment on attachment 170081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170081&action=review > Source/WebCore/bindings/v8/V8Binding.cpp:322 > + if (worldToUse == UseCurrentWorld) { > + if (V8DOMWindowShell* isolatedContext = V8DOMWindowShell::getEntered()) > + return v8::Local<v8::Context>::New(isolatedContext->context()); > + } > + return frame->script()->mainWorldContext(); This seems wrong. If we pass UseCurrentWorld but we're not in a V8 context, then we'll return the main world's context. Perhaps this function should crash if we're not in a context? The underlying problem is that the operation this function is trying to perform is impossible. It's not possible to convert a ScriptExecutionContext to a v8::Context because there is a one-to-many relationship between ScriptExecutionContext and v8::Context.

Comment on attachment 170081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170081&action=review >> Source/WebCore/bindings/v8/V8Binding.cpp:322 >> + return frame->script()->mainWorldContext(); > > This seems wrong. If we pass UseCurrentWorld but we're not in a V8 context, then we'll return the main world's context. Perhaps this function should crash if we're not in a context? > > The underlying problem is that the operation this function is trying to perform is impossible. It's not possible to convert a ScriptExecutionContext to a v8::Context because there is a one-to-many relationship between ScriptExecutionContext and v8::Context. I'm thinking: a variant of this patch except that instead of this toV8Context(ScriptExecutionContext, WorldToUse) there is a static function in IDBBindingUtilities that does a subset - given a ScriptExecutionContext, assume it's a Document or WorkerContext (which is true for IDB), which would be the equivalent of always calling this method with UseCurrentWorld. That moves the "risk" to IDB code. And remove the fallback to mainWorldContext() - if there isn't a current context (i.e. this is executing outside a script stack), then return a temporary context via v8::Local<v8::Context>() My confidence in the above is not high, though.

That doesn't really make sense. Talking with Alex now.

Created attachment 171035 [details] Patch Strawman approach

Comment on attachment 171035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171035&action=review > Source/WebCore/ChangeLog:532 > - has its override logical content height computed. > + has its override logical content height computed. Please do no remove trailing whitespace from every other ChangeLog entry.

Please look at your diff before you upload it so it's not full of junk.

Comment on attachment 171035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171035&action=review > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:353 > + ScriptExecutionContext* m_context; Why is having a raw pointer here safe? > Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:58 > +v8::Handle<v8::Context> getIDBV8Context(ScriptExecutionContext* context) > +{ > + if (context) { > + if (context->isDocument()) { > + if (Frame* frame = static_cast<Document*>(context)->frame()) { > + if (V8DOMWindowShell* isolatedContext = V8DOMWindowShell::getEntered()) > + return v8::Local<v8::Context>::New(isolatedContext->context()); > + return frame->script()->mainWorldContext(); > + } > + ASSERT_NOT_REACHED(); This approach is wrong. I've written that several times on this bug. Having a default fallback to the main world is not correct. You need to remember the correct world somehow.

sorry that is just a strawman and I didn't notice that until after I uploaded it. Anyway the point is to get people looking at getV8IDBContext()

Comment on attachment 171035 [details] Patch (removing review flag completely - this wasn't intended to be checked in)

> Anyway the point is to get people looking at getV8IDBContext() Trying to recover the v8::Context from the ScriptExecutionContext does not work. There is a one-to-many relationship between the ScriptExecutionContext and the v8::Context. If you don't have the key for that relation (the DOMWrapperWorld or the WorldContextHandle), you cannot correctly transit from a ScriptExecutionContext to a v8::Context.

One of the sources of confusion is that these functions get used in four "contexts": (1) script calls into an IDB function with a ScriptValue, which performs "extract" operations on that ScriptValue, then serializes it for storage (2) a serialized script value is passed into an IDB function at the conclusion of an asynchronous lookup operation; it is deserialized back to a ScriptValue, then it is returned to script via an event callback. (3) a serialized script value is passed into an IDB function at the conclusion of an asynchronous lookup operation; it is deserialized back to a ScriptValue, then "inject" operations are done to the ScriptValue, and then it is delivered to script via an event callback. (4) an obscure case: script calls into an IDB function which starts an asynchronous sequence of operations involving lookup, deserialize, and "extract", entirely within C++ code. FWIW, the stacks we're seeing point at #2 or #3 as being the "problematic" case. It sounds like we may need to be providing that correct v8::Context in #2/#3, during the deserialization and further steps.

(In reply to comment #19) > > (2) a serialized script value is passed into an IDB function at the conclusion of an asynchronous lookup operation; it is deserialized back to a ScriptValue, then it is returned to script via an event callback. Clarification: it is deserialized to a ScriptValue then stored as a property on an object, accessed through a getter. Previous iterations of the code used a SerializedScriptValue which was deserialized on access; in that case, the deserialization would take place using the context of the caller.

(In reply to comment #19) > It sounds like we may need to be providing that correct v8::Context in #2/#3, during the deserialization and further steps. ... and it's unclear to me what that should be, in theory. Assume it's an IDBRequest that's handling the async result, trying to deserialized a SerializedScriptValue into a ScriptValue. What is the correct world for that? In older code, the SSV would be deserialized on attribute access, so the world was on the stack (UseCurrentWorld). This would lead to distinct values being produced on each access. I can imagine the case where an IDBRequest is passed between worlds (window->iframe), and accessing the attribute causes a deserializtion within each world, leading to objects with distinct prototype chains. In the newer code... does the IDBRequest (via wrappers) have a world? Or would it have wrappers in each world? If the latter, it seems like this approach is fundamentally flawed unless we can pick one world (the one where the IDBRequest is created?)

> ... and it's unclear to me what that should be, in theory. Assume it's an IDBRequest that's handling the async result, trying to deserialized a SerializedScriptValue into a ScriptValue. What is the correct world for that? Whichever world created the IDBRequest. > In older code, the SSV would be deserialized on attribute access, so the world was on the stack (UseCurrentWorld). This would lead to distinct values being produced on each access. I can imagine the case where an IDBRequest is passed between worlds (window->iframe), Moving from a window to an iframe does not change from one world to another. It's probably not possible for an IDBRequest to move from one world to another. Perhaps the IDBRequest should remember which world created it and then ASSERT that it is never accessed in another one? > and accessing the attribute causes a deserializtion within each world, leading to objects with distinct prototype chains. You're describing a problem that occurs when you get the ScriptExecutionContext wrong, not necessarily related to isolated worlds. > In the newer code... does the IDBRequest (via wrappers) have a world? Or would it have wrappers in each world? A given wrapper exists only in a single world. In principle, C++ objects can be exposed to multiple worlds. For example, DOM nodes are often exposed to multiple worlds. However, for many objects, there simply isn't an API that causes multiple worlds to be able to access the same C++ object. It just depends on the API. > If the latter, it seems like this approach is fundamentally flawed unless we can pick one world (the one where the IDBRequest is created?) We can study the API, but I suspect that IDBRequests can only ever be accessed from the world in which they were created. They should probably remember that world and then ASSERT that they are never accessed in another world.

Created attachment 171192 [details] Patch

(In reply to comment #23) > Created an attachment (id=171192) [details] > Patch After review of the API a little, it seems that IDBRequest is the place where the context is lost, so I've stored it and used it when IDBRequest is called back. Unfortunately, the IDB code doesn't have any good place to inject platform specific features, or at least none that I saw, so I've just used ifdefs for now. IDBBIndingUtilities isn't great for this as we need some scoped objects on the stack. I can workaround this later, but this patch should be sufficient to show what needs to be done, which is to ensure that the stored context is on the stack before calling any of those IDBBIndingUtilities functions which need a context.

Comment on attachment 171192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171192&action=review > Source/WebCore/ChangeLog:12 > + * Modules/indexeddb/IDBCursor.cpp: I believe similar changes would be needed in IDBCursor.cpp/.h to capture/set up the scope before calls to idbKeyToScriptValue() but they're not present in the patch. Did they get lost?

(In reply to comment #24) > (In reply to comment #23) > > Created an attachment (id=171192) [details] [details] > > Patch > > After review of the API a little, it seems that IDBRequest is the place where the context is lost, so I've stored it and used it when IDBRequest is called back. Unfortunately, the IDB code doesn't have any good place to inject platform specific features, or at least none that I saw, so I've just used ifdefs for now. IDBBIndingUtilities isn't great for this as we need some scoped objects on the stack. I can workaround this later, but this patch should be sufficient to show what needs to be done, which is to ensure that the stored context is on the stack before calling any of those IDBBIndingUtilities functions which need a context. This patch looks great, at least functionality-wise - IDBRequest is indeed the right place to capture the world. It sounds like we just need a new abstraction on top of the world or the v8 context, and we can take that up elsewhere (I'm happy to do the work)

Comment on attachment 171192 [details] Patch This is a good short-term fix. In the long term, we should make IDBRequest a subclass of a bindings-specific object, like we do with Callback.

(In reply to comment #25) > (From update of attachment 171192 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171192&action=review > > > Source/WebCore/ChangeLog:12 > > + * Modules/indexeddb/IDBCursor.cpp: > > I believe similar changes would be needed in IDBCursor.cpp/.h to capture/set up the scope before calls to idbKeyToScriptValue() but they're not present in the patch. Did they get lost? I thought they are covered as they are always called with an IDBRequest higher up in the stack, but I could be wrong. Maybe some calls are executed differently. I didn't trace all possible call paths. With all the debug checks enabled though, I didn't hit any assertions in the layout tests.

(In reply to comment #28) > I thought they are covered as they are always called with an IDBRequest higher up in the stack, but I could be wrong. Maybe some calls are executed differently. I didn't trace all possible call paths. With all the debug checks enabled though, I didn't hit any assertions in the layout tests. You're right, the stack is set up correctly in IDBRequest::dispatchEvent which calls into IDBCursor::setValueReady, thanks.

Comment on attachment 171192 [details] Patch Great, sounds like we're ready to land this patch. Thank you all for iterating on the approach.

Comment on attachment 171192 [details] Patch Clearing flags on attachment: 171192 Committed r132845: <http://trac.webkit.org/changeset/132845>

All reviewed patches have been landed. Closing bug.

Reverted r132845 for reason: Broke chromium builds, linker errors from IDBBindingUtilitiesTest Committed r132864: <http://trac.webkit.org/changeset/132864>

See first breaking build on Mac: http://build.chromium.org/p/chromium.webkit/builders/Mac%20Builder%20%28dbg%29/builds/15820

____Ld ../../../../../xcodebuild/Debug/libwebkit.dylib cd /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebKit/chromium setenv MACOSX_DEPLOYMENT_TARGET 10.6 /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebKit/chromium/../../../../../third_party/llvm-build/Release+Asserts/bin/clang++ -arch i386 -dynamiclib -isysroot /Developer/SDKs/MacOSX10.6.sdk -L/Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebKit/chromium/../../../../../xcodebuild/Debug -L/Developer/SDKs/MacOSX10.6.sdk/System/Library/Frameworks -L/Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebKit/chromium/../../../../../xcodebuild/Debug -F/Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebKit/chromium/../../../../../xcodebuild/Debug -filelist /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebKit/chromium/../../../../../xcodebuild/WebKit.build/Debug/webkit.build/Objects-normal/i386/webkit.LinkFileList -install_name @rpath/libwebkit.dylib -Xlinker -rpath -Xlinker @loader_path/. -Xlinker -rpath -Xlinker @loader_path/../../.. -mmacosx-version-min=10.6 -Wl,-search_paths_first /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebkit_platform.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libskia.dylib /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libtranslator_glsl.dylib /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libicuuc.dylib /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libv8.dylib -lwebkit_wtf_support /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebcore_bindings.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebcore_test_support.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libtest_support_base.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libgoogleurl.dylib /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libgtest.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libgmock.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libicudata.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libicui18n.dylib /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libjpeg_turbo.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libpng.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libxml2.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libxslt.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libmodp_b64.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libots.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libchrome_zlib.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libcrnspr.dylib /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libcrnss.dylib /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwtf.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebcore_dom.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libtess.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libyarr.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libiccjpeg.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebp_dec.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebp_dsp.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebp_enc.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebp_utils.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libqcms.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libsqlite3.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libgles2_c_lib.dylib /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libleveldatabase.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libbase.dylib /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libdynamic_annotations.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebcore_html.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebcore_platform.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libharfbuzz-ng.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebcore_platform_geometry.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebcore_remaining.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libv8-i18n.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebcore_rendering.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libwebcore_svg.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libbase_static.a /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/xcodebuild/Debug/libbase_i18n.dylib -framework Accelerate -framework OpenGL -framework AppKit -lWebKitSystemInterfaceLeopardPrivateExtern -framework QuartzCore -framework ApplicationServices -framework Carbon -framework CoreFoundation -framework Foundation -framework IOKit -framework Security -framework CoreServices -prebind -single_module -compatibility_version 1 -current_version 1 -o /Volumes/data/b/build/slave/Mac_Builder__dbg_/build/src/third_party/WebKit/Source/WebKit/chromium/../../../../../xcodebuild/Debug/libwebkit.dylib Undefined symbols: "__ZN6WebKit16FrameTestHelpers20createWebViewAndLoadERKSsbPNS_14WebFrameClientEPNS_13WebViewClientE", referenced from: __ZN12_GLOBAL__N_1L7contextEv in IDBBindingUtilitiesTest.o ld: symbol(s) not found

Created attachment 171437 [details] Patch

Comment on attachment 171437 [details] Patch Fixed shared library mac build.

Comment on attachment 171437 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171437&action=review I think we should land this patch, but we might want to clean up the unit test in a followup patch. > Source/WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp:104 > + static WebView* webView; Rather than having a static WebView, it would be better to create it on the stack for each test. That makes sure we're not leaking information between tests > Source/WebKit/chromium/tests/IDBBindingUtilitiesTest.cpp:110 > + ScriptExecutionContext* context = static_cast<WebFrameImpl*>(webView->mainFrame())->frame()->document(); > + return toV8Context(context, WorldContextHandle(UseCurrentWorld)); Given that you only have the main world in these tests, you can get the context from the API using the mainWorldContext() API.

Comment on attachment 171437 [details] Patch Clearing flags on attachment: 171437 Committed r132922: <http://trac.webkit.org/changeset/132922>

All reviewed patches have been landed. Closing bug.