<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.
Created attachment 89113 [details] patch
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 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).
Attachment 89113 [details] did not build on qt: Build output: http://queues.webkit.org/results/8391073
Created attachment 89147 [details] patch
Attachment 89113 [details] did not build on win: Build output: http://queues.webkit.org/results/8386764
Attachment 89147 [details] did not build on gtk: Build output: http://queues.webkit.org/results/8392496
Comment on attachment 89147 [details] patch r=me, gtk seems to have failed on its own
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
Oops, I broke a test. Patch to fix it coming.
Created attachment 89299 [details] patch to fix layout test
Created attachment 89300 [details] patch to fix layout test
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.
Created attachment 89311 [details] patch to fix layout test
layout test fixed http://trac.webkit.org/projects/webkit/changeset/83668
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 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.
Attachment 89311 [details] was posted by a committer and has review+, assigning to Stephanie Lewis for commit.
was committed http://trac.webkit.org/projects/webkit/changeset/83668