Bug 58280

Summary: SerializedScriptValue shouldn't always throw exceptions
Product: WebKit Reporter: Stephanie Lewis <slewis>
Component: WebCore JavaScriptAssignee: Stephanie Lewis <slewis>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, eric, gns, rniwa, slewis, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
oliver: review-
patch
oliver: review+
patch to fix layout test
none
patch to fix layout test
none
patch to fix layout test oliver: review+

Description Stephanie Lewis 2011-04-11 15:43:00 PDT
<rdar://problem/9252824> javascript in an inconsistent state due to serialization returning an un-handled exception

SerializedScriptValue is used in several places that don't call back into javascript.  The callers should decide whether or not to throw exceptions.
Comment 1 Stephanie Lewis 2011-04-11 16:07:21 PDT
Created attachment 89113 [details]
patch
Comment 2 WebKit Review Bot 2011-04-11 16:10:18 PDT
Attachment 89113 [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/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/loader/FrameLoader.h:42:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/js/SerializedScriptValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/bindings/js/SerializedScriptValue.h:54:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/js/SerializedScriptValue.h:54:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/js/SerializedScriptValue.h:54:  Missing spaces around =  [whitespace/operators] [4]
Source/WebCore/bindings/js/SerializedScriptValue.h:67:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/js/SerializedScriptValue.h:67:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/bindings/js/SerializedScriptValue.h:67:  Missing spaces around =  [whitespace/operators] [4]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1259:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1318:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1346:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1393:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1422:  Non-label code inside switch statements should be indented.  [whitespace/indent] [4]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1422:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1424:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1425:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1427:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1428:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1430:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1431:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/bindings/js/SerializedScriptValue.cpp:1434:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1239:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1240:  Tab found; better to use spaces  [whitespace/tab] [1]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 25 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Oliver Hunt 2011-04-11 16:16:15 PDT
Comment on attachment 89113 [details]
patch

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

Rather than a bool i'd prefer
enum SerializationErrorMode { Throwing, NonThrowing }

as that makes things more explicit.

The use of Global will also fail on ToT as geoff has renamed everything, i believe you'll want heap/Strong.h and Strong<..>

r-ing due to the style errors, and it failing to build on tot :-(

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1435
> +    switch (code) {
> +    case SuccessfullyCompleted:
> +    	break;
> +    case StackOverflowError:
> +    	throwError(exec, createStackOverflowError(exec));
> +    	break;
> +    case InterruptedExecutionError:
> +    	throwError(exec, createInterruptedExecutionException(&exec->globalData()));
> +    	break;
> +    case ValidationError:
> +    	throwError(exec, createTypeError(exec, "Unable to deserialize data."));
> +    	break;
> +    case ExistingExceptionError:
> +    case UnspecifiedError:        
> +    	break;
> +    }

I don't like this code, i'd prefer:
if (code == SuccessfullyCompleted)
    return;
switch (code) {
case ...:
   throwError(exec, ...)
   return;
default:
   return;

Also UnspecifiedError and ExistingExceptionError should both create a generic exception message (as we apparently don't know what has gone wrong).
Comment 4 Early Warning System Bot 2011-04-11 16:29:36 PDT
Attachment 89113 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8391073
Comment 5 Stephanie Lewis 2011-04-11 19:44:14 PDT
Created attachment 89147 [details]
patch
Comment 6 Build Bot 2011-04-11 20:00:22 PDT
Attachment 89113 [details] did not build on win:
Build output: http://queues.webkit.org/results/8386764
Comment 7 Gustavo Noronha (kov) 2011-04-12 11:28:16 PDT
Attachment 89147 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8392496
Comment 8 Oliver Hunt 2011-04-12 14:28:21 PDT
Comment on attachment 89147 [details]
patch

r=me, gtk seems to have failed on its own
Comment 9 Stephanie Lewis 2011-04-12 14:52:26 PDT
gtk failure was fallout from removing a header from SerializedScriptValue.h.  It just needed a header include.

committed http://trac.webkit.org/projects/webkit/changeset/83645
Comment 10 Stephanie Lewis 2011-04-12 16:10:03 PDT
Oops, I broke a test.  Patch to fix it coming.
Comment 11 Stephanie Lewis 2011-04-12 16:16:25 PDT
Created attachment 89299 [details]
patch to fix layout test
Comment 12 Stephanie Lewis 2011-04-12 16:17:51 PDT
Created attachment 89300 [details]
patch to fix layout test
Comment 13 Oliver Hunt 2011-04-12 16:21:57 PDT
Comment on attachment 89300 [details]
patch to fix layout test

r=me, but the =0/=1 are unnecessary, and i want a follow up patch that removes the implicit checks that obviously must currently exist.
Comment 14 Stephanie Lewis 2011-04-12 17:09:18 PDT
Created attachment 89311 [details]
patch to fix layout test
Comment 15 Stephanie Lewis 2011-04-12 17:14:16 PDT
layout test fixed http://trac.webkit.org/projects/webkit/changeset/83668
Comment 16 Ryosuke Niwa 2011-06-18 10:13:10 PDT
Is this bug fixed?  If also, please close the bug and clear r+ flags so that it won't show up on the commit queue.
Comment 17 Eric Seidel (no email) 2011-06-18 13:23:15 PDT
Comment on attachment 89300 [details]
patch to fix layout test

Cleared Oliver Hunt's review+ from obsolete attachment 89300 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 18 Eric Seidel (no email) 2011-06-18 13:43:08 PDT
Attachment 89311 [details] was posted by a committer and has review+, assigning to Stephanie Lewis for commit.
Comment 19 Stephanie Lewis 2011-06-18 19:17:20 PDT
was committed http://trac.webkit.org/projects/webkit/changeset/83668