Summary: | JSCell::classInfo() shouldn't have a bunch of mitigations for being called during destruction | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, joepeck, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer | ||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Filip Pizlo
2017-01-15 09:55:25 PST
Created attachment 298898 [details]
the patch
Comment on attachment 298898 [details]
the patch
Doesn't look ready based on EWS. I'll run layout tests locally.
Comment on attachment 298898 [details] the patch Attachment 298898 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2895044 Number of test failures exceeded the failure limit. Created attachment 298902 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 298898 [details] the patch Attachment 298898 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2895040 Number of test failures exceeded the failure limit. Created attachment 298903 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 298898 [details] the patch Attachment 298898 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2895039 Number of test failures exceeded the failure limit. Created attachment 298904 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 298898 [details] the patch Attachment 298898 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2895046 Number of test failures exceeded the failure limit. Created attachment 298906 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Oh yeah, this is all about that silly JSObjectGetPrivate. I have a really brutal fix... Created attachment 298909 [details]
more
Created attachment 298943 [details]
possible patch
Comment on attachment 298943 [details] possible patch Attachment 298943 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2898101 Number of test failures exceeded the failure limit. Created attachment 298944 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 298943 [details] possible patch Attachment 298943 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2898096 Number of test failures exceeded the failure limit. Created attachment 298945 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 298943 [details] possible patch Attachment 298943 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2898104 Number of test failures exceeded the failure limit. Created attachment 298946 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 298943 [details] possible patch Attachment 298943 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2898108 Number of test failures exceeded the failure limit. Created attachment 298947 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 298970 [details]
more
Fixed a bunch of crash bugs.
Comment on attachment 298970 [details] more Attachment 298970 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2900098 New failing tests: fast/dom/DOMURL/searchparams.html fast/text/font-face-set-document.html imported/w3c/web-platform-tests/html/browsers/browsing-the-web/read-media/pageload-video.html imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html imported/w3c/web-platform-tests/fetch/api/request/request-init-002.html fast/text/font-face-set-javascript.html Created attachment 298979 [details]
Archive of layout-test-results from ews117 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 298980 [details]
more
Created attachment 298984 [details]
more
Fixed build and a C API bug
Created attachment 298992 [details]
possible patch
Two changes:
- More changelog.
- Streamlined logic in JSObjectGetPrivate(). I think it's more correct, but who knows. Will wait for EWS before r?.
Created attachment 299000 [details]
the patch
Comment on attachment 299000 [details]
the patch
r=me, although you're killing me with the timing of this.
Comment on attachment 299000 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=299000&action=review r=me > Source/JavaScriptCore/ChangeLog:20 > + being destructed. JSObjectGetPrivate() is UB anyway if you use it on any UB -> undefined behavior > Source/JavaScriptCore/ChangeLog:31 > + That was the only really weird of this patch. The rest is mostly removing illegal Sentence wording. (In reply to comment #30) > Comment on attachment 299000 [details] > the patch > > r=me, although you're killing me with the timing of this. :-( I thought it was a blocker for one of the crashes I had in bug 165760. This ended up not being the reason, but I only realized that after getting over the "no classInfo from destructor" hump. This may have caused some crashes on the bots: https://build.webkit.org/builders/Apple%20Sierra%20Release%20JSC%20%28Tests%29/builds/427 > ** The following JSC stress test failures have been introduced: > stress/reflect-enumerate.js.ftl-eager > stress/typedarray-of.js.no-cjit-collect-continuously Failures looked like: > stress/reflect-enumerate.js.ftl-eager: test_script_14719: line 2: 43319 Trace/BPT trap: 5 ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --useFTLJIT\=true --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 --thresholdForOptimizeAfterWarmUp\=20 --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 --maximumEvalCacheableSourceLength\=150000 --useEagerCodeBlockJettisonTiming\=true --collectContinuously\=true --useGenerationalGC\=false reflect-enumerate.js ) > stress/reflect-enumerate.js.ftl-eager: ERROR: Unexpected exit code: 133 > stress/typedarray-of.js.no-cjit-collect-continuously: test_script_18860: line 2: 80608 Trace/BPT trap: 5 ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 --scribbleFreeCells\=true --collectContinuously\=true --useGenerationalGC\=false typedarray-of.js ) > stress/typedarray-of.js.no-cjit-collect-continuously: ERROR: Unexpected exit code: 133 (In reply to comment #33) > This may have caused some crashes on the bots: > https://build.webkit.org/builders/ > Apple%20Sierra%20Release%20JSC%20%28Tests%29/builds/427 > > > ** The following JSC stress test failures have been introduced: > > stress/reflect-enumerate.js.ftl-eager > > stress/typedarray-of.js.no-cjit-collect-continuously > > Failures looked like: > > > stress/reflect-enumerate.js.ftl-eager: test_script_14719: line 2: 43319 Trace/BPT trap: 5 ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --useFTLJIT\=true --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 --thresholdForOptimizeAfterWarmUp\=20 --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 --maximumEvalCacheableSourceLength\=150000 --useEagerCodeBlockJettisonTiming\=true --collectContinuously\=true --useGenerationalGC\=false reflect-enumerate.js ) > > stress/reflect-enumerate.js.ftl-eager: ERROR: Unexpected exit code: 133 > > > stress/typedarray-of.js.no-cjit-collect-continuously: test_script_18860: line 2: 80608 Trace/BPT trap: 5 ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 --scribbleFreeCells\=true --collectContinuously\=true --useGenerationalGC\=false typedarray-of.js ) > > stress/typedarray-of.js.no-cjit-collect-continuously: ERROR: Unexpected exit code: 133 Darn. I'm momentarily afk, and it's cool to roll this out. (In reply to comment #34) > (In reply to comment #33) > > This may have caused some crashes on the bots: > > https://build.webkit.org/builders/ > > Apple%20Sierra%20Release%20JSC%20%28Tests%29/builds/427 > > > > > ** The following JSC stress test failures have been introduced: > > > stress/reflect-enumerate.js.ftl-eager > > > stress/typedarray-of.js.no-cjit-collect-continuously > > > > Failures looked like: > > > > > stress/reflect-enumerate.js.ftl-eager: test_script_14719: line 2: 43319 Trace/BPT trap: 5 ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --useFTLJIT\=true --thresholdForJITAfterWarmUp\=10 --thresholdForJITSoon\=10 --thresholdForOptimizeAfterWarmUp\=20 --thresholdForOptimizeAfterLongWarmUp\=20 --thresholdForOptimizeSoon\=20 --thresholdForFTLOptimizeAfterWarmUp\=20 --thresholdForFTLOptimizeSoon\=20 --maximumEvalCacheableSourceLength\=150000 --useEagerCodeBlockJettisonTiming\=true --collectContinuously\=true --useGenerationalGC\=false reflect-enumerate.js ) > > > stress/reflect-enumerate.js.ftl-eager: ERROR: Unexpected exit code: 133 > > > > > stress/typedarray-of.js.no-cjit-collect-continuously: test_script_18860: line 2: 80608 Trace/BPT trap: 5 ( "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFunctionDotArguments\=true --maxPerThreadStackUsage\=1572864 --useConcurrentJIT\=false --thresholdForJITAfterWarmUp\=100 --scribbleFreeCells\=true --collectContinuously\=true --useGenerationalGC\=false typedarray-of.js ) > > > stress/typedarray-of.js.no-cjit-collect-continuously: ERROR: Unexpected exit code: 133 > > Darn. I'm momentarily afk, and it's cool to roll this out. Looking at this now. This landed in http://trac.webkit.org/changeset/210821 Keeping open while I double-check that it's at fault for crashes. (In reply to comment #36) > This landed in http://trac.webkit.org/changeset/210821 > > Keeping open while I double-check that it's at fault for crashes. I'm pretty sure that this is the cause of crashes. I'll roll it out. Rolled out in http://trac.webkit.org/changeset/210824 (In reply to comment #38) > Rolled out in http://trac.webkit.org/changeset/210824 It's clear that rolling this out fixed the crashes but I still cannot repro those darn crashes locally. I'll keep trying. There's also this: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r210822%20(10432)/plugins/destroy-plugin-from-callback-crash-log.txt Some NPAPI stuff needs to stop using jsCast in some places. Boy, the crash rate of the crash the bot saw is incredibly low. I got it to crash on my machine once. It seems to consistently crash in assertions in checkOffsetConsistency, so I've made those assertions more verbose. I haven't seen a crash since. I'm simultaneously running tests on bot157 since that one seemed to get the crash once per run-javascriptcore-tests run. It's more than half-way through a test run right now and still no crash. I made a small change to the assertion to cover a race condition in the assert itself. Now I cannot get a crash anymore. I've done two runs in a row on bot157 without a crash and I've done a bunch of runs on my machine without crashing. I'll do one more round of testing and if no crash then I'll reland. Created attachment 299076 [details]
patch for relanding
Relanded in https://trac.webkit.org/changeset/210829 |