Bug 88451 – Extra HandleScope in V8Proxy

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eriq Augustine]

Extra HandleScope in V8Proxy

[Eriq Augustine]

Created attachment 146102 [details]
Patch

[WebKit Review Bot]

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.

[Eriq Augustine]

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.

[Adam Barth]

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?

[Adam Barth]

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?

[Adam Barth]

> 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

[Adam Barth]

> 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

[Eriq Augustine]

Created attachment 146137 [details]
Patch

[Eriq Augustine]

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

[Adam Barth]

Comment on attachment 146137 [details]
Patch

That's certainly very pretty.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Eriq Augustine]

Created attachment 146372 [details]
Patch

[Eriq Augustine]

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.

[Adam Barth]

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

[Eriq Augustine]

(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

[Adam Barth]

Comment on attachment 146372 [details]
Patch

Ok.

[WebKit Review Bot]

Comment on attachment 146372 [details]
Patch

Clearing flags on attachment: 146372

Committed r120229: <http://trac.webkit.org/changeset/120229>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.