Bug 164998 – Fix exception scope verification failures in runtime/Error* files.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Mark Lam]

Created attachment 295295 [details]
proposed patch.

[Darin Adler]

Comment on attachment 295295 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/ErrorConstructor.cpp:58
> +    scope.release();

I must admit I don’t understand why this is needed here, but the same line of code is not needed in the new ErrorInstance::create function below.

> Source/JavaScriptCore/runtime/ErrorInstance.cpp:46
> +    String messageStr = message.isUndefined() ? String() : message.toString(exec)->value(exec);

Would be better to name the argument state instead of exec.

Should use toWTFString instead of toString(exec)->value(exec).

Should not use the abbreviation "str", so please call this function messageString.

[Mark Lam]

Comment on attachment 295295 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/ErrorConstructor.cpp:58
>> +    scope.release();
> 
> I must admit I don’t understand why this is needed here, but the same line of code is not needed in the new ErrorInstance::create function below.

Two reasons:

1. For now, we're not aiming to exhaustively add ThrowScopes (and thereby necessitate scope.release()s) at all places in the code.  Instead, we're driving their addition incrementally as needed.  In my case, I'm running the JSC tests with JSC_validateExceptionChecks=true, and fixing any failures that the validator finds.

    So, if there isn't already a test case that exercises that code path, then we won't detect the need to add the scope.release() and won't add it for now.
    Alternatively, if we already know for sure that that code path can throw even if it's not currently covered by our tests, we'll go ahead and pre-emptively add the scope.release().

2. The ThrowScope need not necessarily be added at every level that an exception may propagate through.  For example, let's say we have the following functions:

    void foo()
    {
        throwException(...);
    }

    void bar(int someValue)
    {
        foo();
    }

    void goo() {
        // Does not throw anything.
    }

    void baz()
    {
        if (reasons) {
             bar(1);
             Should check for exception here.
        }
        if (reasons2) {
            // Should release throw scope here before tail call.
            bar(2);
            return;
        }
        // No need for scope release here.
        goo();
    }

foo() needs a ThrowScope because it throws.
bar() does not need a ThrowScope because it is a simple function the exception merely passes through.
baz() needs a ThrowScope because it is not a simple function: it can call bar(1) which may throw, and that exception needs to checked before we fall through to calling bar(2).  We need a scope.release() before bar(2) so that the ThrowScope in baz will know that the exception returned by bar(2) need not be checked, and that we'll simply propagate it to our caller.

Note that the call to goo() does not need to be preceded with a scope.release() because goo() does not throw any exceptions.

Note also that because bar() does not need a ThrowScope, it also does not need a scope.release().

>> Source/JavaScriptCore/runtime/ErrorInstance.cpp:46
>> +    String messageStr = message.isUndefined() ? String() : message.toString(exec)->value(exec);
> 
> Would be better to name the argument state instead of exec.
> 
> Should use toWTFString instead of toString(exec)->value(exec).
> 
> Should not use the abbreviation "str", so please call this function messageString.

Fixed.

[Mark Lam]

Created attachment 295300 [details]
patch for landing.

Thanks for the review.  Will wait for EWS for to run it through before I commit the patch.

[Build Bot]

Comment on attachment 295300 [details]
patch for landing.

Attachment 295300 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2552606

New failing tests:
js/kde/evil-n.html
sputnik/Conformance/15_Native_Objects/15.11_Error/15.11.1/S15.11.1.1_A1_T1.html
inspector/unit-tests/async-test-suite.html
imported/w3c/web-platform-tests/dom/nodes/DOMImplementation-createDocument.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-bogus-value.htm
inspector/unit-tests/sync-test-suite.html
js/dom/stack-at-creation-for-error-objects.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-bogus-name.htm
imported/w3c/web-platform-tests/dom/events/EventTarget-dispatchEvent.html
streams/reference-implementation/transform-stream.html
js/dom/native-error-prototype.html
js/dom/webidl-type-mapping.html
sputnik/Conformance/15_Native_Objects/15.11_Error/15.11.2/S15.11.2.1_A1_T1.html
js/dom/js-correct-exception-handler.html
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html

[Build Bot]

Created attachment 295305 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 295300 [details]
patch for landing.

Attachment 295300 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2552632

New failing tests:
js/kde/evil-n.html
sputnik/Conformance/15_Native_Objects/15.11_Error/15.11.1/S15.11.1.1_A1_T1.html
inspector/unit-tests/async-test-suite.html
imported/w3c/web-platform-tests/dom/nodes/DOMImplementation-createDocument.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-bogus-value.htm
inspector/unit-tests/sync-test-suite.html
js/dom/stack-at-creation-for-error-objects.html
imported/w3c/web-platform-tests/XMLHttpRequest/setrequestheader-bogus-name.htm
imported/w3c/web-platform-tests/dom/events/EventTarget-dispatchEvent.html
streams/reference-implementation/transform-stream.html
js/dom/native-error-prototype.html
js/dom/webidl-type-mapping.html
sputnik/Conformance/15_Native_Objects/15.11_Error/15.11.2/S15.11.2.1_A1_T1.html
js/dom/js-correct-exception-handler.html
imported/w3c/web-platform-tests/webrtc/rtcpeerconnection/rtcpeerconnection-constructor.html

[Build Bot]

Created attachment 295306 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Mark Lam]

Comment on attachment 295300 [details]
patch for landing.

Fix coming.

[Mark Lam]

Created attachment 295307 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 295307 [details]
Patch for landing.

Clearing flags on attachment: 295307

Committed r208952: <http://trac.webkit.org/changeset/208952>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.