Bug 197693 - JSObject::getOwnPropertyDescriptor is missing an exception check
Summary: JSObject::getOwnPropertyDescriptor is missing an exception check
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
: 197485 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-05-08 09:43 PDT by Tadeu Zagallo
Modified: 2019-05-22 13:49 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.53 KB, patch)
2019-05-08 09:50 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (3.10 MB, application/zip)
2019-05-08 11:29 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews212 for win-future (13.62 MB, application/zip)
2019-05-08 12:20 PDT, EWS Watchlist
no flags Details
Patch (3.51 KB, patch)
2019-05-08 14:18 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (13.61 MB, application/zip)
2019-05-08 17:32 PDT, EWS Watchlist
no flags Details
Patch (6.94 KB, patch)
2019-05-13 02:21 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-05-08 09:43:17 PDT
<rdar://problem/50441784>
Comment 1 Tadeu Zagallo 2019-05-08 09:50:58 PDT
Created attachment 369392 [details]
Patch
Comment 2 Saam Barati 2019-05-08 10:25:38 PDT
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
Comment 3 EWS Watchlist 2019-05-08 11:29:32 PDT
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
Comment 4 EWS Watchlist 2019-05-08 11:29:33 PDT
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
Comment 5 EWS Watchlist 2019-05-08 12:20:53 PDT
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
Comment 6 EWS Watchlist 2019-05-08 12:20:56 PDT
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
Comment 7 Tadeu Zagallo 2019-05-08 14:18:38 PDT
Created attachment 369421 [details]
Patch
Comment 8 Tadeu Zagallo 2019-05-08 14:20:25 PDT
(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.
Comment 9 Saam Barati 2019-05-08 16:27:54 PDT
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.
Comment 10 EWS Watchlist 2019-05-08 17:32:28 PDT
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
Comment 11 EWS Watchlist 2019-05-08 17:32:39 PDT
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
Comment 12 Tadeu Zagallo 2019-05-09 02:16:19 PDT
(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.
Comment 13 Saam Barati 2019-05-09 10:17:41 PDT
(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.
Comment 14 Tadeu Zagallo 2019-05-13 02:21:57 PDT
Created attachment 369719 [details]
Patch
Comment 15 Saam Barati 2019-05-13 11:44:12 PDT
Comment on attachment 369719 [details]
Patch

r=me
Comment 16 WebKit Commit Bot 2019-05-13 13:52:10 PDT
Comment on attachment 369719 [details]
Patch

Clearing flags on attachment: 369719

Committed r245249: <https://trac.webkit.org/changeset/245249>
Comment 17 WebKit Commit Bot 2019-05-13 13:52:12 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Shawn Roberts 2019-05-14 14:02:23 PDT
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
Comment 19 Michael Saboff 2019-05-22 13:49:58 PDT
*** Bug 197485 has been marked as a duplicate of this bug. ***