Bug 30774 – [V8] Missing null check after string conversion in error reporting

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Mads Ager]

When reporting exceptions, we sometimes perform a string conversion on an exception object.  That string conversion can fail if an exception is thrown during conversion.  In that case an empty handle is returned.  We need to check for that.

See comment 1 and 3 of http://crbug.com/24130.

[Mads Ager]

Created attachment 41863 [details]
Add null check.

[Adam Barth]

Comment on attachment 41863 [details]
Add null check.

Test?

[Mads Ager]

A test would indeed be great.  I haven't been able to reproduce this crash yet, but there are a number of crashes reported where we get into this situation.  I'll spent some more time tomorrow attempting to reproduce.

[Adam Barth]

You can't just throw an exception in the toString?

[Mads Ager]

Yeah, that is not the problem.  The problem is getting into this case in the code first.  I need to be in a situation where the TryCatch has no message *and* the ToString operations throws an exception.  The first part is the hard part - most of the time a TryCatch should have a message.  I need something like a stack overflow at an unfortunate time to hit this path.

[Adam Barth]

So, I'm supposed to lean on you to write a test, but if you complain enough that it's hard to write the test, then we can go ahead with the patch anyway.

In any case, you don't need me to lecture you about testing.  :)

[Mads Ager]

Created attachment 41942 [details]
Add null check and test case.

[Adam Barth]

Comment on attachment 41942 [details]
Add null check and test case.

Beautiful patch.  Thanks!

[WebKit Commit Bot]

Comment on attachment 41942 [details]
Add null check and test case.

Rejecting patch 41942 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Adam Barth', '--force']" exit_code: 2
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/bindings/v8/V8Utilities.cpp
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/error-to-string-stack-overflow-expected.txt
Died at /Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply line 438, <> line 109.

[Eric Seidel (no email)]

Investigating the strange failure...

[Eric Seidel (no email)]

    } elsif (isGit()) {
        system "git", "add", $path or die;
    }

I'm just not sure why it died yet, but I will investigate.

[Eric Seidel (no email)]

My apologies.  False rejection.  I'm working on a fix to svn-apply right now.  bug 30826

[Eric Seidel (no email)]

Comment on attachment 41942 [details]
Add null check and test case.

svn-apply fixed.  Trying again.

[WebKit Commit Bot]

Comment on attachment 41942 [details]
Add null check and test case.

Clearing flags on attachment: 41942

Committed r50160: <http://trac.webkit.org/changeset/50160>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.