Bug 30904 - [v8] Missing IsEmpty() check in V8Proxy::sourceName()
Summary: [v8] Missing IsEmpty() check in V8Proxy::sourceName()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-29 05:51 PDT by Yury Semikhatsky
Modified: 2009-10-30 06:42 PDT (History)
5 users (show)

See Also:

Attachments
patch (8.23 KB, patch)
2009-10-29 10:43 PDT, Yury Semikhatsky 		no flags Details | Formatted Diff | Diff
patch (8.00 KB, patch)
2009-10-29 10:52 PDT, Yury Semikhatsky 		no flags Details | Formatted Diff | Diff
long running test (2.01 KB, patch)
2009-10-29 11:42 PDT, Yury Semikhatsky 		no flags Details | Formatted Diff | Diff
test case that completes in a reasonable time (2.03 KB, patch)
2009-10-30 06:25 PDT, Yury Semikhatsky 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2009-10-29 05:51:46 PDT 
V8Proxy::sourceName() calls 'frameSourceName' JavaScript function to retrieve current source name. This call may cause stack overflow in which case result handle is empty. We should check that the result is not an empty handle before converting it to a String.
Comment 1 Yury Semikhatsky 2009-10-29 10:43:09 PDT 
Created attachment 42110 [details]
patch

I would like to add a layout test for this patch but it needs to overflow JavaScript stack and takes for about 30 seconds to complete on my machine. I'm going to provide some means for setting JavaScript stack limit from layout tests first and after that commit the test case itself. I'll post the patch with the long running test case a bit later in a separate patch so that you can see what I'm talking about.

Since the patch is fixing one of the most frequent current Chromium crashes I'd prefer to submit it sooner and add the test later.
Comment 2 Yury Semikhatsky 2009-10-29 10:52:54 PDT 
Created attachment 42112 [details]
patch

Previous patch with commented code removed.
Comment 3 Yury Semikhatsky 2009-10-29 11:42:45 PDT 
Created attachment 42118 [details]
long running test 

Here is the test that I was talking about.
Comment 4 Eric Seidel (no email) 2009-10-29 13:57:27 PDT 
Assuming this does not require any special 2-sided coordination, I'm happy to add this to the commit-queue.  As far as I can tell Yury is not a committer yet.
Comment 5 Pavel Feldman 2009-10-29 14:23:50 PDT 
(In reply to comment #4)
> Assuming this does not require any special 2-sided coordination, I'm happy to
> add this to the commit-queue.  As far as I can tell Yury is not a committer
> yet.

He was willing to test it on Chrome Mac first. I'll do it for him tomorrow if he is fine with it. Patch looks right to me and is tested on Windows.
Comment 6 Yury Semikhatsky 2009-10-30 03:33:26 PDT 
Password for 'yurys@chromium.org': 
	M	WebCore/ChangeLog
	M	WebCore/bindings/scripts/CodeGeneratorV8.pm
	M	WebCore/bindings/v8/ScriptCallStack.cpp
	M	WebCore/bindings/v8/ScriptCallStack.h
	M	WebCore/bindings/v8/V8Proxy.cpp
	M	WebCore/bindings/v8/V8Proxy.h
	M	WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp
Committed r50327
Comment 7 Yury Semikhatsky 2009-10-30 06:25:03 PDT 
Created attachment 42208 [details]
test case that completes in a reasonable time

Layout test that completes in a reasonable time. Thanks olliej for his suggestion!
Comment 8 Yury Semikhatsky 2009-10-30 06:41:57 PDT 
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/dom/console-log-stack-overflow-expected.txt
	A	LayoutTests/fast/dom/console-log-stack-overflow.html
Committed r50331