Bug 189227 – The watchdog sometimes fails to terminate a script.

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

[Mark Lam]

<rdar://problem/39932857>

[Mark Lam]

Created attachment 348728 [details]
proposed patch.

[EWS Watchlist]

Comment on attachment 348728 [details]
proposed patch.

Attachment 348728 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/9067847

New failing tests:
stress/regress-189227-watchdog-on-infinite-loop.js.dfg-eager
stress/regress-189227-watchdog-on-infinite-loop.js.default
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-small-pool
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-eager-no-cjit-b3o1
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-validate-sampling-profiler
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-eager-no-cjit
stress/regress-189227-watchdog-on-infinite-loop.js.no-ftl
stress/regress-189227-watchdog-on-infinite-loop.js.dfg-eager-no-cjit-validate
stress/regress-189227-watchdog-on-infinite-loop.js.no-cjit-collect-continuously
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-b3o1
stress/regress-189227-watchdog-on-infinite-loop.js.no-cjit-validate-phases
stress/regress-189227-watchdog-on-infinite-loop.js.no-llint
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-no-inline-validate
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-no-put-stack-validate
apiTests

[Saam Barati]

Comment on attachment 348728 [details]
proposed patch.

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

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2853
> +    if (UNLIKELY(vm.needTrapHandling()))

If you make CheckTraps exit to check_traps, you won’t need this code

[Mark Lam]

Created attachment 348733 [details]
proposed patch.

Adjusted test heuristics to hopefully eliminate flakiness.

[EWS Watchlist]

Comment on attachment 348733 [details]
proposed patch.

Attachment 348733 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/9071026

New failing tests:
stress/regress-189227-watchdog-on-infinite-loop.js.dfg-eager
stress/regress-189227-watchdog-on-infinite-loop.js.dfg-eager-no-cjit-validate
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-small-pool
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-eager-no-cjit-b3o1
stress/regress-189227-watchdog-on-infinite-loop.js.no-cjit-validate-phases
stress/regress-189227-watchdog-on-infinite-loop.js.no-ftl
stress/regress-189227-watchdog-on-infinite-loop.js.no-cjit-collect-continuously
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-b3o1
stress/regress-189227-watchdog-on-infinite-loop.js.no-llint
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-no-inline-validate
stress/regress-189227-watchdog-on-infinite-loop.js.ftl-no-cjit-no-put-stack-validate
apiTests

[Saam Barati]

Are you going to make the change where you fix how check_traps is lowered in the bytecode parser?

[Mark Lam]

(In reply to Saam Barati from comment #6)
> Are you going to make the change where you fix how check_traps is lowered in
> the bytecode parser?

Yes, I should take this patch out of review

[Mark Lam]

Created attachment 348785 [details]
proposed patch.

Let's get some EWS testing.

[Mark Lam]

Comment on attachment 348785 [details]
proposed patch.

The EWS is still consistently reproducing a failure on the new test.  I'm going to investigate this first.

[Mark Lam]

Created attachment 348786 [details]
proposed patch.

Looks like the issue is that on some bots, the watchdog thread can't get a time slice before the main thread runs to completion.  Let's make the test timeout ridiculously large to give it adequate margin.

[Mark Lam]

Comment on attachment 348786 [details]
proposed patch.

This margin is still insufficient.  In the end, I cannot make this test not racy.  I have to make the test a true infinite loop and rely on the jsc test harness to timeout the test if it hangs.  New patch coming soon.

[Mark Lam]

Created attachment 348787 [details]
proposed patch.

[Saam Barati]

Comment on attachment 348787 [details]
proposed patch.

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3967
> +        compileCheckTraps(node);

You should also call noResult here just for ensuring good style in case CheckTraps ever gets any child nodes

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:4448
> +        compileCheckTraps(node);

Ditto

[Mark Lam]

Comment on attachment 348787 [details]
proposed patch.

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

Thanks for the review.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3967
>> +        compileCheckTraps(node);
> 
> You should also call noResult here just for ensuring good style in case CheckTraps ever gets any child nodes

I'll add the noResult(node) call to the implementation of compileCheckTraps().

[Mark Lam]

Created attachment 348794 [details]
patch for landing.

[WebKit Commit Bot]

Comment on attachment 348794 [details]
patch for landing.

Clearing flags on attachment: 348794

Committed r235605: <https://trac.webkit.org/changeset/235605>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.