Bug 58280 – SerializedScriptValue shouldn't always throw exceptions

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Stephanie Lewis]

<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.

[Stephanie Lewis]

Created attachment 89113 [details]
patch

[WebKit Review Bot]

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.

[Oliver Hunt]

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).

[Early Warning System Bot]

Attachment 89113 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8391073

[Stephanie Lewis]

Created attachment 89147 [details]
patch

[Build Bot]

Attachment 89113 [details] did not build on win:
Build output: http://queues.webkit.org/results/8386764

[Gustavo Noronha (kov)]

Attachment 89147 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8392496

[Oliver Hunt]

Comment on attachment 89147 [details]
patch

r=me, gtk seems to have failed on its own

[Stephanie Lewis]

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

[Stephanie Lewis]

Oops, I broke a test.  Patch to fix it coming.

[Stephanie Lewis]

Created attachment 89299 [details]
patch to fix layout test

[Stephanie Lewis]

Created attachment 89300 [details]
patch to fix layout test

[Oliver Hunt]

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.

[Stephanie Lewis]

Created attachment 89311 [details]
patch to fix layout test

[Stephanie Lewis]

layout test fixed http://trac.webkit.org/projects/webkit/changeset/83668

[Ryosuke Niwa]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Attachment 89311 [details] was posted by a committer and has review+, assigning to Stephanie Lewis for commit.

[Stephanie Lewis]

was committed http://trac.webkit.org/projects/webkit/changeset/83668