Bug 93646

Created attachment 157527 [details] initial patch to show direction of change

Comment on attachment 157527 [details] initial patch to show direction of change this patch works in the sense that it compiles and the example extension in http://code.google.com/p/chromium/issues/detail?id=20773 works. It's nowhere near cleaned up though, and I've probably introduced some memory leaks.

Created attachment 157696 [details] mostly the same as first patch rebased after V8PerContextData rename

Created attachment 160690 [details] Patch

Comment on attachment 160690 [details] Patch Attachment 160690 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13616143 New failing tests: http/tests/appcache/different-origin-manifest.html accessibility/aria-checkbox-checked.html accessibility/aria-labelledby-on-input.html http/tests/appcache/document-write-html-element.html animations/3d/matrix-transform-type-animation.html accessibility/aria-readonly.html http/tests/appcache/cyrillic-uri.html accessibility/canvas-fallback-content.html animations/animation-add-events-in-handler.html http/tests/appcache/deferred-events-delete-while-raising.html accessibility/accessibility-node-reparent.html http/tests/appcache/fail-on-update-2.html animations/animation-direction-alternate-reverse.html http/tests/appcache/access-via-redirect.php accessibility/aria-hidden.html http/tests/appcache/destroyed-frame.html http/tests/appcache/disabled.html animations/animation-controller-drt-api.html animations/3d/transform-perspective.html accessibility/aria-scrollbar-role.html http/tests/appcache/crash-when-navigating-away-then-back.html canvas/philip/tests/2d.canvas.reference.html animations/3d/state-at-end-event-transform.html accessibility/aria-labelledby-stay-within.html animations/animation-direction-reverse-hardware.html accessibility/aria-help.html animations/animation-direction-reverse-fill-mode.html accessibility/adjacent-continuations-cause-assertion-failure.html animations/animation-direction-reverse-timing-functions-hardware.html canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

Created attachment 160694 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 160690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160690&action=review Looks like you have some test failures. This patch looks very promising. I need to study it in more detail. Is it possible to make this change in smaller steps? There's a lot to digest in this patch. > Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:69 > + v8::Persistent<v8::Context> localCopy = v8::Persistent<v8::Context>::New(context); Can we store this Persistent handle as a ScopedPersistent on DOMWrapperWorld?

Comment on attachment 160690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=160690&action=review > Source/WebCore/bindings/v8/ScriptController.cpp:338 > + m_isolatedWorlds.set(worldId, isolatedWorldShell.get()); Can we key m_isolatedWorlds off of DOMWrapperWorld rather than worldId? I'd like to remove the concept of worldId from WebCore eventually and have that just be a concern of the WebKit-layer (i.e., code in Source/WebKit/chromium). > Source/WebCore/bindings/v8/ScriptController.cpp:443 > + isolatedShell = windowShell(isolatedShell->world()); > + // FIXME: need to set security token here > + isolatedShell->initContextIfNeeded(); For example, this work can be in a separate patch. This is a big behavior change, and it would be nice to make that change separately from all the refactorings that make it possible. > Source/WebCore/bindings/v8/ScriptController.h:69 > V8DOMWindowShell* windowShell() const { return m_windowShell.get(); } We probably want to delete this function and have all the callers use windowShell(mainWold()) or whatever. > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:90 > +static v8::Handle<v8::Object> getGlobalObject(v8::Handle<v8::Context> context) > +{ > + return v8::Handle<v8::Object>::Cast(context->Global()->GetPrototype()); > +} getGlobalObject is a bit of a misnomer. How about toInnerGlobalObject(v8::Handle<v8::Context>) > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:97 > +V8DOMWindowShell* V8DOMWindowShell::isolatedWorldContext() This function should have the term "entered" in its name somewhere because it's calling v8::Context::GetEntered > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:220 > + : m_frame(frame), > + m_world(world), These commas go on the next line under the ":". See Other Punctuation in http://www.webkit.org/coding/coding-style.html > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:298 > +static void isolatedContextWeakCallback(v8::Persistent<v8::Value> object, void* parameter) > +{ > + object.Dispose(); > + reinterpret_cast<V8DOMWindowShell*>(parameter)->deref(); > +} > + > +static void registerWeakHandler(v8::Handle<v8::Context> context, V8DOMWindowShell* shell) > +{ > + v8::Persistent<v8::Context>::New(context).MakeWeak(shell, isolatedContextWeakCallback); > + shell->ref(); > +} Can we hold this Persistent handle as a ScopedPersistent on V8DOMWindowShell ? Doesn't it already have a ScopedPersistent to a v8::Context?

(In reply to comment #7) > (From update of attachment 160690 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=160690&action=review > > Looks like you have some test failures. This patch looks very promising. I need to study it in more detail. > > Is it possible to make this change in smaller steps? There's a lot to digest in this patch. It's not so easy. There are lots of refactors after this that need to be done, but they all assume the isolated context is gone, and I'm not sure I can really do that in multiple steps. I moved the patch to https://bugs.webkit.org/show_bug.cgi?id=95735 since it no longer fixes this issue.

Created attachment 175963 [details] Patch

Comment on attachment 175963 [details] Patch This patch currently handles all user scripts except those injected by WebView.addUserScript, which I believe may just be apps, but I'm not sure. All extensions that I invent seem to work correctly.

That's great! I will study the patch carefully. Does this mean http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/DOMWrapperWorld.cpp#L85 is now called multiple times per DOMWrapperWorld, based on the number of Frames that the world gets booted up in? If you've already gone to sleep, I can puzzle that out for myself.

(In reply to comment #12) > That's great! I will study the patch carefully. > > Does this mean http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/v8/DOMWrapperWorld.cpp#L85 is now called multiple times per DOMWrapperWorld, based on the number of Frames that the world gets booted up in? If you've already gone to sleep, I can puzzle that out for myself. it's pretty early to go to sleep. i'll explain: i've avoided dealing with the types of worlds that have called makeContextWeak in this patch, so they don't have cross frame access yet. I'll deal with that in a different patch. I wanted to confer with you on that first. We have 2 options: get rid of the weak contexts which could mean a lot more mem consumption for whatever actually uses them do a complicated thing where the a new hidden object in javascript that represents the DOMWrapperWorld has a weak handle on the context and the context a weak handle on the world js object so that they all die at once I'm inclined to do the second, but it would be a lot easier just to remove a few lines of code than do a bunch of v8 api magic to make this work wdyt?

Comment on attachment 175963 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175963&action=review > Source/WebCore/bindings/v8/ScriptController.cpp:459 > + // FIXME: Need to handle weak isolated worlds correctly. This is where i explicit stop the weak contexts from being accessible. Otherwise the frame context or it's parent could die in background and would be reinit'ed with an new window context which would be bad.

Comment on attachment 175963 [details] Patch This patch is great and exactly how I'd hoped we'd be able to implement this feature. My only real comment is stylistic. I'd add a helper function align the lines of inline v8::Local<v8::Context> ScriptController::contextForWorld(DOMWrapperWorld* world) { return v8::Local<v8::Context>::New(windowShell(world)->context()); } since you use that idiom a bunch. Once this rolls into Chromium, would you be willing to add extension API test so we can test the integration with the extension system?

(In reply to comment #15) > (From update of attachment 175963 [details]) > This patch is great and exactly how I'd hoped we'd be able to implement this feature. > > My only real comment is stylistic. I'd add a helper function align the lines of > > inline v8::Local<v8::Context> ScriptController::contextForWorld(DOMWrapperWorld* world) > { > return v8::Local<v8::Context>::New(windowShell(world)->context()); > } Okay, will add. I was inlining those since i figured the first couple branches at least got hit a lot in the perf tests. > > since you use that idiom a bunch. > > Once this rolls into Chromium, would you be willing to add extension API test so we can test the integration with the extension system? sure. i've put it on my todo list.

> Okay, will add. I was inlining those since i figured the first couple branches at least got hit a lot in the perf tests. You can ask the compiler to inline it rather than doing it manually.

Comment on attachment 175963 [details] Patch I'll add that inline change tomorrow, to avoid the timezone wait.

Comment on attachment 175963 [details] Patch Thanks.

Comment on attachment 175963 [details] Patch Clearing flags on attachment: 175963 Committed r135765: <http://trac.webkit.org/changeset/135765>

All reviewed patches have been landed. Closing bug.

http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r135791%20(4011)/results.html http/tests/security/isolatedWorld/world-reuse.html failing on Apple Mac and Windows ports after this patch.

We should probably update the baseline for now and then have Dan look at why it might be failing on JSC.

(In reply to comment #23) > We should probably update the baseline for now and then have Dan look at why it might be failing on JSC. Looking into it

(In reply to comment #23) > We should probably update the baseline for now and then have Dan look at why it might be failing on JSC. I've submitted a patch that fixes the test to pass on safari and chrome builds. https://bugs.webkit.org/show_bug.cgi?id=103385 it looks like that if you run evaluateScriptInIsolatedWorld in safari, it's run in the main window's frame's context. If you do the same on chrome, it runs in the context of the current frame.