Bug 197693 – JSObject::getOwnPropertyDescriptor is missing an exception check

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

[Tadeu Zagallo]

<rdar://problem/50441784>

[Tadeu Zagallo]

Created attachment 369392 [details]
Patch

[Saam Barati]

Comment on attachment 369392 [details]
Patch

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

This looks identical to a bug Michael was looking into that caused other issues. Might be worth verifying with what the other issues were.

> Source/JavaScriptCore/runtime/JSObject.cpp:3448
> +    ASSERT(!scope.exception() || !result);

EXCEPTION_ASSERT instead

[EWS Watchlist]

Comment on attachment 369392 [details]
Patch

Attachment 369392 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12134379

New failing tests:
http/tests/security/cross-frame-access-getOwnPropertyDescriptor.html

[EWS Watchlist]

Created attachment 369399 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 369392 [details]
Patch

Attachment 369392 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12135051

New failing tests:
http/tests/css/filters-on-iframes.html

[EWS Watchlist]

Created attachment 369407 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Tadeu Zagallo]

Created attachment 369421 [details]
Patch

[Tadeu Zagallo]

(In reply to Saam Barati from comment #2)
> Comment on attachment 369392 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369392&action=review
> 
> This looks identical to a bug Michael was looking into that caused other
> issues. Might be worth verifying with what the other issues were.

Yep, looks like the same issue and the exact same fix, which explains the test failures on my patch. I think it should be correct now.

[Saam Barati]

Comment on attachment 369421 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSObject.cpp:3447
> +    bool result = methodTable(vm)->getOwnPropertySlot(this, exec, propertyName, slot);

Our policy was if this throws, it must return false. Is that no longer true? Who broke that? We can remove a branch if this is the case.

[EWS Watchlist]

Comment on attachment 369421 [details]
Patch

Attachment 369421 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12138241

New failing tests:
svg/repaint/remove-border-property-on-root.html

[EWS Watchlist]

Created attachment 369450 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Tadeu Zagallo]

(In reply to Saam Barati from comment #9)
> Comment on attachment 369421 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369421&action=review
> 
> > Source/JavaScriptCore/runtime/JSObject.cpp:3447
> > +    bool result = methodTable(vm)->getOwnPropertySlot(this, exec, propertyName, slot);
> 
> Our policy was if this throws, it must return false. Is that no longer true?
> Who broke that? We can remove a branch if this is the case.

That's no longer true. JSLocation::getOwnPropertySlot returns true in case of exception. I guess it would be better to just fix that instead? I'll update the patch.

[Saam Barati]

(In reply to Tadeu Zagallo from comment #12)
> (In reply to Saam Barati from comment #9)
> > Comment on attachment 369421 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=369421&action=review
> > 
> > > Source/JavaScriptCore/runtime/JSObject.cpp:3447
> > > +    bool result = methodTable(vm)->getOwnPropertySlot(this, exec, propertyName, slot);
> > 
> > Our policy was if this throws, it must return false. Is that no longer true?
> > Who broke that? We can remove a branch if this is the case.
> 
> That's no longer true. JSLocation::getOwnPropertySlot returns true in case
> of exception. I guess it would be better to just fix that instead? I'll
> update the patch.

I think that would be better.

[Tadeu Zagallo]

Created attachment 369719 [details]
Patch

[Saam Barati]

Comment on attachment 369719 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 369719 [details]
Patch

Clearing flags on attachment: 369719

Committed r245249: <https://trac.webkit.org/changeset/245249>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Roberts]

After changes in https://trac.webkit.org/changeset/245249

We are seeing 32 failures on the Debug JSC queue

** The following JSC stress test failures have been introduced:
	stress/proxy-delete.js.bytecode-cache
	stress/proxy-delete.js.default
	stress/proxy-delete.js.dfg-eager
	stress/proxy-delete.js.dfg-eager-no-cjit-validate
	stress/proxy-delete.js.dfg-maximal-flush-validate-no-cjit
	stress/proxy-delete.js.ftl-eager
	stress/proxy-delete.js.ftl-eager-no-cjit
	stress/proxy-delete.js.ftl-eager-no-cjit-b3o1
	stress/proxy-delete.js.ftl-no-cjit-b3o0
	stress/proxy-delete.js.ftl-no-cjit-no-inline-validate
	stress/proxy-delete.js.ftl-no-cjit-no-put-stack-validate
	stress/proxy-delete.js.ftl-no-cjit-small-pool
	stress/proxy-delete.js.ftl-no-cjit-validate-sampling-profiler
	stress/proxy-delete.js.no-cjit-validate-phases
	stress/proxy-delete.js.no-ftl
	stress/proxy-delete.js.no-llint
	stress/proxy-property-descriptor.js.bytecode-cache
	stress/proxy-property-descriptor.js.default
	stress/proxy-property-descriptor.js.dfg-eager
	stress/proxy-property-descriptor.js.dfg-eager-no-cjit-validate
	stress/proxy-property-descriptor.js.dfg-maximal-flush-validate-no-cjit
	stress/proxy-property-descriptor.js.ftl-eager
	stress/proxy-property-descriptor.js.ftl-eager-no-cjit
	stress/proxy-property-descriptor.js.ftl-eager-no-cjit-b3o1
	stress/proxy-property-descriptor.js.ftl-no-cjit-b3o0
	stress/proxy-property-descriptor.js.ftl-no-cjit-no-inline-validate
	stress/proxy-property-descriptor.js.ftl-no-cjit-no-put-stack-validate
	stress/proxy-property-descriptor.js.ftl-no-cjit-small-pool
	stress/proxy-property-descriptor.js.ftl-no-cjit-validate-sampling-profiler
	stress/proxy-property-descriptor.js.no-cjit-validate-phases
	stress/proxy-property-descriptor.js.no-ftl
	stress/proxy-property-descriptor.js.no-llint

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2788/steps/jscore-test/logs/stdio

[Michael Saboff]

*** Bug 197485 has been marked as a duplicate of this bug. ***