Bug 167066 – JSCell::classInfo() shouldn't have a bunch of mitigations for being called during destruction

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Every time you use JSCell::classInfo(), you pay a compile time, code size, and running time cost for mitigations that are there to make that method safe to call from a destructor.

The only reason why we call that method in destructors is:

- Invalid use of jsCast<>.  We used to have the discipline to only use static_cast<> in destructors.
- Some isolated uses of the classInfo in specific subclasses.  Those can do the mitigations themselves.

We should also assert in classInfo() that we aren't destructing.

[Filip Pizlo]

Created attachment 298898 [details]
the patch

[Filip Pizlo]

Comment on attachment 298898 [details]
the patch

Doesn't look ready based on EWS.  I'll run layout tests locally.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Filip Pizlo]

Oh yeah, this is all about that silly JSObjectGetPrivate.

I have a really brutal fix...

[Filip Pizlo]

Created attachment 298909 [details]
more

[Filip Pizlo]

Created attachment 298943 [details]
possible patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Filip Pizlo]

Created attachment 298970 [details]
more

Fixed a bunch of crash bugs.

[Build Bot]

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

[Build Bot]

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

[Filip Pizlo]

Created attachment 298980 [details]
more

[Filip Pizlo]

Created attachment 298984 [details]
more

Fixed build and a C API bug

[Filip Pizlo]

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?.

[Filip Pizlo]

Created attachment 299000 [details]
the patch

[Radar WebKit Bug Importer]

<rdar://problem/30044439>

[Keith Miller]

Comment on attachment 299000 [details]
the patch

r=me, although you're killing me with the timing of this.

[Michael Saboff]

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.

[Filip Pizlo]

(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.

[Joseph Pecoraro]

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

[Filip Pizlo]

(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.

[Filip Pizlo]

(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.

[Filip Pizlo]

This landed in http://trac.webkit.org/changeset/210821

Keeping open while I double-check that it's at fault for crashes.

[Filip Pizlo]

(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.

[Filip Pizlo]

Rolled out in http://trac.webkit.org/changeset/210824

[Filip Pizlo]

(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.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 299076 [details]
patch for relanding

[Filip Pizlo]

Relanded in https://trac.webkit.org/changeset/210829