Extra HandleScope in V8Proxy
Created attachment 146102 [details] Patch
Attachment 146102 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:11: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 1 files If any of these errors are false positives, please file a bug against check-webkit-style.
I have not been able to get a test to expose this error in WebKit alone, however I have got it to crash chromium. Regardless, I believe that there is a clear problem here. In V8Proxy::evaluateInIsolatedWorld(), the extra HandleScope will cause any Values placed into |results| to be destroyed with the HandleScope. The V8Proxy is only called from the v8 ScriptController, and a HandleScope is created there. The ScriptController then wraps any Value in a ScriptValue which internally uses a Persistent Handle, ensuring the safety of the Values. An alternative to just deleting the HandleScope would be to add the ability for a HandleScope to Close() around multiple Handles.
Comment on attachment 146102 [details] Patch I see, the problem is that we're trying to return local handles in |results|. Which function above this one is responsible for having a HandleScope?
I see what you've written in Comment #3. Can you be more specific about the functions involved? I can grep around the codebase, but that will delay feedback from me on this patch.
What about making the result a v8::Array instead of a WTF::Vector?
> What about making the result a v8::Array instead of a WTF::Vector? You'd still need to have a v8::Local<v8::Array>. Wouldn't that have the same problem?
(In reply to comment #7) > > What about making the result a v8::Array instead of a WTF::Vector? > > You'd still need to have a v8::Local<v8::Array>. Wouldn't that have the same problem? You can return a single handle from a handle scope. The v8::Array in turn would reference all the handles to the results, and so everything stays alive
> You can return a single handle from a handle scope. The v8::Array in turn would reference all the handles to the results, and so everything stays alive Woah, crazy. How is the lifetime of that handle managed? Maybe that only works if there's a HandleScope above you to manage the lifetime of the handle? I guess it depends on whether this HandleScope is redundant. If someone higher up on the stack is always holding a HandleScope, then probably should just delete this one.
(In reply to comment #9) > > You can return a single handle from a handle scope. The v8::Array in turn would reference all the handles to the results, and so everything stays alive > > Woah, crazy. How is the lifetime of that handle managed? Maybe that only works if there's a HandleScope above you to manage the lifetime of the handle? Yes, it requires an outer handle scope. > I guess it depends on whether this HandleScope is redundant. If someone higher up on the stack is always holding a HandleScope, then probably should just delete this one. It seems less brittle to have a handlescope close to where you create handles, so it's clear what is supposed to survive and what is local only
Created attachment 146137 [details] Patch
(In reply to comment #10) > It seems less brittle to have a handlescope close to where you create handles, so it's clear what is supposed to survive and what is local only I agree. I went with the v8::Array solution.
Comment on attachment 146137 [details] Patch That's certainly very pretty.
Comment on attachment 146137 [details] Patch Attachment 146137 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12919135 New failing tests: userscripts/window-onerror-for-isolated-world-1.html userscripts/window-onerror-for-isolated-world-2.html
Created attachment 146256 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 146372 [details] Patch
Comment on attachment 146372 [details] Patch Added a little safeguard incase evaluate returns an empty handle. I think v8::Array has a problem with being passed an empty handle.
Comment on attachment 146372 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146372&action=review > Source/WebCore/bindings/v8/V8Proxy.cpp:263 > + if (evaluationResult.IsEmpty()) > + evaluationResult = v8::Local<v8::Value>::New(v8::Undefined()); Does this line make sense? I thought empty handles and v8::Undefined where the same...
(In reply to comment #18) > (From update of attachment 146372 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146372&action=review > > > Source/WebCore/bindings/v8/V8Proxy.cpp:263 > > + if (evaluationResult.IsEmpty()) > > + evaluationResult = v8::Local<v8::Value>::New(v8::Undefined()); > > Does this line make sense? I thought empty handles and v8::Undefined where the same... I don't think they act the same (at least in the context of v8:Arrays). I looked at how v8:Array Sets and it just dereferences the handle which asserts if the handle is empty
Comment on attachment 146372 [details] Patch Ok.
Comment on attachment 146372 [details] Patch Clearing flags on attachment: 146372 Committed r120229: <http://trac.webkit.org/changeset/120229>
All reviewed patches have been landed. Closing bug.