Bug 181400 – REGRESSION (r226485): Many new wasm leaks detected by the leaks bot

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexey Proskuryakov]

The number of leaks detected has substantially increased with r226485. Perhaps leaks checking doesn't work with poisoning?

[Radar WebKit Bug Importer]

<rdar://problem/36358768>

[Alexey Proskuryakov]

Created attachment 331941 [details]
disable poisoning on the bot

Sounds like the short term solution is to disable poisoning on the bot. JF, is this likely to continue working for now?

[JF Bastien]

Comment on attachment 331941 [details]
disable poisoning on the bot

(In reply to Alexey Proskuryakov from comment #2)
> Created attachment 331941 [details]
> disable poisoning on the bot
> 
> Sounds like the short term solution is to disable poisoning on the bot. JF,
> is this likely to continue working for now?

Disabling poisoning should remain a reliable manner to avoid leaks. It looks like you're disabling it only for the leaks bots, which is important since we want to test with poisoning elsewhere.

If the leaks program changes in the future, say to ignore the WebKit zones, we should make sure that with JSC_usePoisoning=0 they do detect leaks properly (otherwise we'd just ignore the leaks).

[WebKit Commit Bot]

Comment on attachment 331941 [details]
disable poisoning on the bot

Clearing flags on attachment: 331941

Committed r227342: <https://trac.webkit.org/changeset/227342>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This did not work, I needed to wrap the additional argument.

[Alexey Proskuryakov]

Created attachment 332169 [details]
do it right

[Sam Weinig]

Comment on attachment 332169 [details]
do it right

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

> Tools/ChangeLog:6
> +        https://bugs.webkit.org/show_bug.cgi?id=181400
> +        https://bugs.webkit.org/show_bug.cgi?id=181400
> +

Double URL

[Alexey Proskuryakov]

Comment on attachment 332169 [details]
do it right

Wait, this still doesn't make sense. This should be built into run-webkit-tests, so that checking for leaks works for engineers.

[Alexey Proskuryakov]

Created attachment 332176 [details]
do it right

Okay, I hope that it finally makes sense.

[Joseph Pecoraro]

Comment on attachment 332176 [details]
do it right

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

rs=me

> Tools/BuildSlaveSupport/build.webkit.org-config/config.json:133
>                      { "name": "Apple High Sierra (Leaks)", "type": "TestWebKit1Leaks", "builddir": "highsierra-leaks",
>                        "platform": "mac-highsierra", "configuration": "release", "architectures": ["x86_64"],
> -                      "additionalArguments": ["--no-retry-failures", "--no-sample-on-timeout", "--JSC_usePoisoning=0"],
> +                      "additionalArguments": ["--no-retry-failures", "--no-sample-on-timeout"],
>                        "slavenames": ["bot121"]

Wondering why I don't see the obvious --leaks and -1 on this "TestWebKit1Leaks" bot. Must be buried somewhere else?

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:433
> +    if options.leaks:
> +        options.additional_env_var.append("JSC_usePoisoning=0")

Is leaks intended to work with WebKit2? If so, you should probably also include "__XPC_JSC_usePoisoning=0" as well, so that WebContentProcesses would get it. In practice I think we only run --leaks with -1 right now.

[Alexey Proskuryakov]

> Wondering why I don't see the obvious --leaks and -1 on this "TestWebKit1Leaks" bot. Must be buried somewhere else?

Correct, all bots with type TestWebKit1Leaks get that. Maybe "--no-retry-failures", "--no-sample-on-timeout" should be there too, not sure.

> Is leaks intended to work with WebKit2?

It doesn't work with multi-process in a meaningful way (cf. bug 167851). But I'll add the XPC variant, so that we have fewer things to fix for WebKit2 in the future.

[Alexey Proskuryakov]

Committed http://trac.webkit.org/r227547