Bug 53594 – [v8] frame several more JS code invocations into v8::TryCatch

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[anton muhin]

[v8] frame several more JS code invocations into v8::TryCatch

[anton muhin]

I am changing v8::TryCatch semantics to allow it to catch exceptions thrown from embedder code (see http://codereview.chromium.org/6397011/).  This patch prepares v8 bindings to properly handle new semantics (but we will need minor LayoutTests rebaselining as well).

[anton muhin]

Created attachment 80913 [details]
Patch

[Adam Barth]

Comment on attachment 80913 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Covered by the existing layout tests.

What do you mean by that?  Will we pass some tests that we used to fail after this patch?  If so, we should list them.  If not, we should write tests that show the change in behavior.

[anton muhin]

(In reply to comment #3)
> (From update of attachment 80913 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80913&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Covered by the existing layout tests.
> 
> What do you mean by that?  Will we pass some tests that we used to fail after this patch?  If so, we should list them.  If not, we should write tests that show the change in behavior.

I mean that this patch doesn't change current behaviour (at least as far as Layout Tests are concerned).  But it is a necessary preparation step before I land v8 patch.  Does that sound reasonable?  May you suggest the proper wording?

[Adam Barth]

Comment on attachment 80913 [details]
Patch

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

>>> Source/WebCore/ChangeLog:8
>>> +        Covered by the existing layout tests.
>> 
>> What do you mean by that?  Will we pass some tests that we used to fail after this patch?  If so, we should list them.  If not, we should write tests that show the change in behavior.
> 
> I mean that this patch doesn't change current behaviour (at least as far as Layout Tests are concerned).  But it is a necessary preparation step before I land v8 patch.  Does that sound reasonable?  May you suggest the proper wording?

Maybe I'm just lacking context on this change.  Do you mean its not possible to throw exceptions that will be caught by these blocks?  It's probably better to explain why this patch doesn't change any observable behavior and, give that's the case, why we want to make this change.  If it's at all possible to observe this change, we'll want to add a test for it, even though no such test exists currently.  Almost every patch should have an associated test.  It's the rare exception that a patch won't have a test.

[anton muhin]

I am sorry, Adam, I didn't explain it properly.  Yes, there is a context.

Currently there is a bug in v8: if the exception is thrown with v8 API (using v8::ThrowException), it's not caught by v8::TryCatch unless v8 crosses C++/JS boundary.  For most of the cases, v8 does cross it.  But sometimes, for example, when query object with GetRealNamedProperty, it doesn't and one can obtain an empty handle to the property while property is actually present.  There is also http://code.google.com/p/v8/issues/detail?id=1072

So we decided to fix it and make such exceptions catchable by surrounding v8::TryCatch.  That though changes exception throwing semantics and hence I needed to prepare some places in v8 bindings to to be committed to v8 change.  Hence this patch.  It doesn't alter v8 bindings behaviour as far as tested by layout tests (save for reasonable rebaseline of a single layout test).

And I cannot guarantee that we won't change behaviour on some paths, but so far the best thing I can do is just run all the tests we have.

Another way to consider this patch: imagine that instead of first submitting patch to WebKit, I'd submit it to v8.  On next v8 roll, several Layout tests would started to fail.  And one would require this patch or something similar to fix them.  So that's kind of preemptive fix.

I hope it explains the reason for this patch, but I'd be glad to give any additional details.

(In reply to comment #5)
> (From update of attachment 80913 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80913&action=review
> 
> >>> Source/WebCore/ChangeLog:8
> >>> +        Covered by the existing layout tests.
> >> 
> >> What do you mean by that?  Will we pass some tests that we used to fail after this patch?  If so, we should list them.  If not, we should write tests that show the change in behavior.
> > 
> > I mean that this patch doesn't change current behaviour (at least as far as Layout Tests are concerned).  But it is a necessary preparation step before I land v8 patch.  Does that sound reasonable?  May you suggest the proper wording?
> 
> Maybe I'm just lacking context on this change.  Do you mean its not possible to throw exceptions that will be caught by these blocks?  It's probably better to explain why this patch doesn't change any observable behavior and, give that's the case, why we want to make this change.  If it's at all possible to observe this change, we'll want to add a test for it, even though no such test exists currently.  Almost every patch should have an associated test.  It's the rare exception that a patch won't have a test.

[Adam Barth]

Comment on attachment 80913 [details]
Patch

Thanks.  That makes a lot of sense.  Can you add some of that information to the ChangeLog before landing?

[anton muhin]

Created attachment 81074 [details]
Addressing Adam's comments

[anton muhin]

Sure, Adam.  May you quickly check the wording of the new patch?

(In reply to comment #7)
> (From update of attachment 80913 [details])
> Thanks.  That makes a lot of sense.  Can you add some of that information to the ChangeLog before landing?

[Adam Barth]

Comment on attachment 81074 [details]
Addressing Adam's comments

Looks great!  Thanks.

[anton muhin]

Comment on attachment 81074 [details]
Addressing Adam's comments

Thanks a lot for review, Adam!

[WebKit Commit Bot]

Comment on attachment 81074 [details]
Addressing Adam's comments

Rejecting attachment 81074 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 1

Last 500 characters of output:
rks/Python.framework/Versions/2.6/lib/python2.6/urllib2.py", line 361, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_http.py", line 756, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_http.py", line 706, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/7689919

[WebKit Commit Bot]

Comment on attachment 81074 [details]
Addressing Adam's comments

Rejecting attachment 81074 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'land-a..." exit_code: 1

Last 500 characters of output:
ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

Full output: http://queues.webkit.org/results/7693834

[Adam Barth]

Created attachment 81187 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 81187 [details]
Patch for landing

Rejecting attachment 81187 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'land-a..." exit_code: 1

Last 500 characters of output:
ommit-queue/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 64, in _run
    step(tool, options).run(state)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 57, in run
    if self._has_valid_reviewer(changelog_entry):
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 42, in _has_valid_reviewer
    if changelog_entry.reviewer():
AttributeError: 'NoneType' object has no attribute 'reviewer'

Full output: http://queues.webkit.org/results/7698428

[Adam Barth]

Ah!  I figured it out.  Your ChangeLog indent is off by one.

[Adam Barth]

Created attachment 81192 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 81192 [details]
Patch for landing

Clearing flags on attachment: 81192

Committed r77613: <http://trac.webkit.org/changeset/77613>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77613 might have broken GTK Linux 64-bit Debug

[anton muhin]

(In reply to comment #16)
> Ah!  I figured it out.  Your ChangeLog indent is off by one.

Thanks a lot for your help, Adam!