[V8] SerializedScriptValue should handle JS exceptions
Created attachment 82637 [details] patch
Attachment 82637 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/bindings/v8/SerializedScriptValue.cpp:493: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/v8/SerializedScriptValue.cpp:687: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1229: An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Total errors found: 3 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 82638 [details] patch (style errors fixed)
Comment on attachment 82638 [details] patch (style errors fixed) View in context: https://bugs.webkit.org/attachment.cgi?id=82638&action=review LGTM > Source/WebCore/ChangeLog:3 > + Reviewed by Mihai Parparita. is it indeed reviewed? > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1240 > didThrow = true; shouldn't you throw some exception then?
Anton, thanks for the comments! (In reply to comment #4) > (From update of attachment 82638 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=82638&action=review > > LGTM > > > Source/WebCore/ChangeLog:3 > > + Reviewed by Mihai Parparita. > > is it indeed reviewed? I hope Mihai can have a look :) > > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1240 > > didThrow = true; > > shouldn't you throw some exception then? We don't know whether it's safe to re-enter V8 here.
(In reply to comment #5) > Anton, thanks for the comments! > > (In reply to comment #4) > > (From update of attachment 82638 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=82638&action=review > > > > LGTM > > > > > Source/WebCore/ChangeLog:3 > > > + Reviewed by Mihai Parparita. > > > > is it indeed reviewed? > > I hope Mihai can have a look :) > > > > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1240 > > > didThrow = true; > > > > shouldn't you throw some exception then? > > We don't know whether it's safe to re-enter V8 here. I think it should be, but I am not insisting.
Comment on attachment 82638 [details] patch (style errors fixed) I'm not 100% comfortable with reviewing this change, since I'm not that familiar with the V8 side of things. View in context: https://bugs.webkit.org/attachment.cgi?id=82638&action=review > LayoutTests/ChangeLog:3 > + Reviewed by Mihai Parparita. Please don't pre-populate this, webkit-patch land and/or the commit queue will pick up the reviewer based on who set the r+ flag. > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:497 > + if (m_propertyNames.IsEmpty()) Does this mean that postMessage({}) will report an error now? What was the the rationale for that change (the structured clone argument seems to allow empty objects). > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1236 > + if (status == Serializer::JSFailure) { You only seem to use JSFailure for empty inputs, so it seems like throwing an exception should be safe here (assuming empty means empty objects, and that is indeed a situation to avoid).
(In reply to comment #7) > (From update of attachment 82638 [details]) Mihai, Thanks for having a look! > I'm not 100% comfortable with reviewing this change, since I'm not that familiar with the V8 side of things. IIRC, you're last one who touched this code (making non-trivial changes). I also added Anton who is now an expert in V8 exception handling. > View in context: https://bugs.webkit.org/attachment.cgi?id=82638&action=review > > > LayoutTests/ChangeLog:3 > > + Reviewed by Mihai Parparita. > > Please don't pre-populate this, webkit-patch land and/or the commit queue will pick up the reviewer based on who set the r+ flag. OK. > > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:497 > > + if (m_propertyNames.IsEmpty()) > > Does this mean that postMessage({}) will report an error now? What was the the rationale for that change (the structured clone argument seems to allow empty objects). This is confusing. IsEmpty() applies to the handle here, not to the underlying object. The rationale for the change is that there are corner cases where the V8 API can return an empty handle (to signal something bad has happened) without throwing an exception, i.e. we can't rely solely on tryCatch.HasCaught(). In these cases the only thing we can do is unwind the C++ stack as it's not safe to re-enter V8. This doesn't matter much now because we'll most likely crash anyway. But the general idea behind this is that if we unwind to the outermost JS frame it may be able to recover and we'll e.g. disable the JS in the current frame but still can continue running it in the others. > > Source/WebCore/bindings/v8/SerializedScriptValue.cpp:1236 > > + if (status == Serializer::JSFailure) { > > You only seem to use JSFailure for empty inputs, so it seems like throwing an exception should be safe here (assuming empty means empty objects, and that is indeed a situation to avoid). Please see the explanation above.
Comment on attachment 82638 [details] patch (style errors fixed) Thanks for the explanations.
M LayoutTests/ChangeLog M LayoutTests/fast/dom/Window/window-postmessage-clone-expected.txt M LayoutTests/fast/dom/Window/window-postmessage-clone.html M Source/WebCore/ChangeLog M Source/WebCore/bindings/v8/SerializedScriptValue.cpp Committed r79209