[Webkit-unassigned] [Bug 54555] [V8] SerializedScriptValue should handle JS exceptions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54555





--- Comment #8 from Vitaly Repeshko <vitalyr at chromium.org>  2011-02-18 13:47:25 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.