Bug 181400 - REGRESSION (r226485): Many new wasm leaks detected by the leaks bot
Summary: REGRESSION (r226485): Many new wasm leaks detected by the leaks bot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-08 13:43 PST by Alexey Proskuryakov
Modified: 2018-01-24 12:49 PST (History)
12 users (show)

See Also:


Attachments
disable poisoning on the bot (1.62 KB, patch)
2018-01-22 10:08 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
do it right (1.67 KB, patch)
2018-01-24 09:00 PST, Alexey Proskuryakov
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
do it right (2.38 KB, patch)
2018-01-24 10:03 PST, Alexey Proskuryakov
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2018-01-08 13:43:08 PST
The number of leaks detected has substantially increased with r226485. Perhaps leaks checking doesn't work with poisoning?
Comment 1 Radar WebKit Bug Importer 2018-01-08 13:44:21 PST
<rdar://problem/36358768>
Comment 2 Alexey Proskuryakov 2018-01-22 10:08:58 PST
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?
Comment 3 JF Bastien 2018-01-22 10:13:22 PST
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).
Comment 4 WebKit Commit Bot 2018-01-22 10:42:54 PST
Comment on attachment 331941 [details]
disable poisoning on the bot

Clearing flags on attachment: 331941

Committed r227342: <https://trac.webkit.org/changeset/227342>
Comment 5 WebKit Commit Bot 2018-01-22 10:42:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Alexey Proskuryakov 2018-01-23 17:56:18 PST
This did not work, I needed to wrap the additional argument.
Comment 7 Alexey Proskuryakov 2018-01-24 09:00:44 PST
Created attachment 332169 [details]
do it right
Comment 9 Alexey Proskuryakov 2018-01-24 09:47:42 PST
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.
Comment 10 Alexey Proskuryakov 2018-01-24 10:03:17 PST
Created attachment 332176 [details]
do it right

Okay, I hope that it finally makes sense.
Comment 11 Joseph Pecoraro 2018-01-24 10:36:29 PST
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.
Comment 12 Alexey Proskuryakov 2018-01-24 12:45:06 PST
> 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.
Comment 13 Alexey Proskuryakov 2018-01-24 12:49:53 PST
Committed http://trac.webkit.org/r227547