[webkit-reviews] review denied: [Bug 174380] [JSC] retain original stacktrace when rethrowing an ErrorInstance : [Attachment 315149] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 11 15:03:12 PDT 2017


Mark Lam <mark.lam at apple.com> has denied Caitlin Potter (:caitp)
<caitp at igalia.com>'s request for review:
Bug 174380: [JSC] retain original stacktrace when rethrowing an ErrorInstance
https://bugs.webkit.org/show_bug.cgi?id=174380

Attachment 315149: Patch

https://bugs.webkit.org/attachment.cgi?id=315149&action=review




--- Comment #10 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 315149
  --> https://bugs.webkit.org/attachment.cgi?id=315149
Patch

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

r- since the bots are red, and also because I suspect this is not the right fix
for the issue.	I would like to understand better what the actual issue is
first.

> Source/JavaScriptCore/ChangeLog:16
> +	   Associate the Exception object from the first time an ErrorInstance
> +	   object has been thrown, and use that original Exception to retain
> +	   the original stacktrace. This is useful when reporting uncaught
> +	   exceptions which have been rethrown at some point.
> +
> +	   This behaviour matches the behaviour of Chromium and Firefox, in
that
> +	   only subclasses of Error retain the stacktrace. Non-ErrorInstance
> +	   exceptions will continue to produce an incorrect stacktrace when
> +	   rethrown.

I do not understand why this is the desired behavior.  The Exception object
captures where the object was last thrown from.  When would the Exception
object's stack not show where the ErrorInstance was thrown from?  Also, if the
ErrorInstance was caught and then rethrown, shouldn't we be showing the new
stack for the new throw location?  Hence, something doesn't add up here.  Can
you please give an example of what error is being observed by the user?  I
suspect your fix is not correct.

> Source/JavaScriptCore/runtime/Exception.cpp:79
> +    if (thrownValue.isObject() && thrownValue.inherits(vm,
ErrorInstance::info())) {
> +	   ErrorInstance* errorInstance = jsCast<ErrorInstance*>(thrownValue);
> +	   errorInstance->setFirstThrownException(vm, this);
> +    }

You can simplify this as:
    if (auto* errorInstance = jsDynamicCast<ErrorInstance>(thrownValue))
	errorInstance->setFirstThrownException(vm, this);


More information about the webkit-reviews mailing list