Bug 102739 – V8: EmptyScriptState passed to functions marked with CallWith=ScriptState

• Component: New Bugs
• Status: RESOLVED FIXED

[Michael Pruett]

Adding CallWith=ScriptState to a function causes the V8 binding generator to invoke the function with an EmptyScriptState rather than with ScriptState::current(). Since the EmptyScriptState has a null v8::Context, any clients which depend upon a valid v8::Context will fail.

[Michael Pruett]

Created attachment 175112 [details]
Patch

This change causes CodeGeneratorV8.pm to pass ScriptState::current() rather than an EmptyScriptState to functions marked with CallWith=ScriptState. With this patch I was able to update IndexedDB to use the ScriptState provided by the binding generator for serialization.

[Adam Barth]

Comment on attachment 175112 [details]
Patch

Great.  Do you want to prepare a proper patch for review (e.g., with a ChangeLog)

[Kentaro Hara]

Comment on attachment 175112 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175112&action=review

The change makes sense.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3529
> -    my @callWithArgs = GenerateCallWith($callWith, \@callWithOutput, $indent, 0, 1, $function);
> +    my @callWithArgs = GenerateCallWith($callWith, \@callWithOutput, $indent, 0, 0, $function);

I think that now no one uses 1. Maybe you can kill the $emptyContext argument from GenerateCallWith().

[Kentaro Hara]

Comment on attachment 175112 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175112&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1633
>          } else {

Then you can remove this if-else branch.

[Michael Pruett]

Created attachment 175115 [details]
Patch

[Kentaro Hara]

Comment on attachment 175115 [details]
Patch

LGTM. If you set r?, I can r+ it.

[Michael Pruett]

Do you know if there is a reason why EmptyScriptState was used in the past in preference to ScriptState::current()?

[Kentaro Hara]

(In reply to comment #7)
> Do you know if there is a reason why EmptyScriptState was used in the past in preference to ScriptState::current()?

Don't know. Would you git blame its history and cc folks?

[Michael Pruett]

Passing EmptyScriptState to functions marked with CallWith=ScriptState seems to have been introduced to CodeGeneratorV8.pm in revision 58298 on 2010-04-27 by Adam Barth. There was no previous use of either EmptyScriptState or ScriptState::current().

[Adam Barth]

I have no idea why I used EmptyScriptState.

[Kentaro Hara]

Either way I think this change looks reasonable.

[Michael Pruett]

Created attachment 175158 [details]
Patch

I have updated the ChangeLog to note that this patch has been reviewed by Kentaro Hara.

[Kentaro Hara]

Comment on attachment 175158 [details]
Patch

From next time, instead of writing my name to the ChangeLog, please set r?. Then a reviewer can set r+ or r-.

[Kentaro Hara]

Comment on attachment 175158 [details]
Patch

And if you want to commit it, please set cq?. (You can find these forms at the right bottom of https://bugs.webkit.org/attachment.cgi?id=175158&action=review)

[WebKit Review Bot]

Comment on attachment 175158 [details]
Patch

Attachment 175158 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14912534

New failing tests:
traversal/exception-forwarding.html

[Michael Pruett]

Created attachment 175245 [details]
Patch

Previously any exception thrown while in EmptyScriptState was cleared when EmptyScriptState went out of scope. I've updated the patch to explicitly clear an exception before throwing it.

[WebKit Review Bot]

Comment on attachment 175245 [details]
Patch

Attachment 175245 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14894690

New failing tests:
fast/dom/shadow/shadow-dom-event-dispatching.html

[Kentaro Hara]

Comment on attachment 175245 [details]
Patch

It looks like the failure is not related to the patch. Let's try again.

[WebKit Review Bot]

Comment on attachment 175245 [details]
Patch

Clearing flags on attachment: 175245

Committed r135325: <http://trac.webkit.org/changeset/135325>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.