1881681 - WPT test harness doesn't treat fatal assertions as test-failures, if they happen after we navigate away from a test

Status: NEW

(Testing :: web-platform-tests, defect)

Description

[Daniel Holbert [:dholbert]]

I've got a crashtest that I'm trying to get landed, for a fixed bug (bug 1833279).

In builds that are affected by the bug, I'd like the crashtest to trip a fatal assertion and turn the harness orange.

In this case, it turns out that the fatal assertion does show up in the log (hooray!) but it doesn't turn the test run orange, apparently because the assertion is after we navigate away from my test. Unfortunately the WPT harness seems to treat this as things being green/just-fine right now.

Here's a Try run, with this WPT being added in a patch that I'm applying on top of an old/buggy commit:
https://treeherder.mozilla.org/jobs?repo=try&revision=1a147d63b8f38dc37019267b0f3e5229410de8e6&searchStr=linux%2Cdebug

In this Try run, all of the Linux Debug Wc runs are green, despite the fact that most of them actually trip this fatal assertion right after navigating away from my crashtest:

Assertion failure: ObserverCount() == mEarlyRunners.Length() (observers, except pending selection scrolls, should have been unregistered), at /builds/worker/checkouts/gecko/layout/base/nsRefreshDriver.cpp:1391

Here's one example log, which is green (Result: success) despite clearly showing this fatal assertion-failure having happened (aborting a process, in this case during ShutdownXPCOM):
https://treeherder.mozilla.org/logviewer?job_id=448165198&repo=try&lineNumber=30296

I might be able to mess with my WPT to force the assertion to happen during the WPT itself, but I'm not entirely sure. Regardless, it seems like we should be treating fatal assertions (aborts) as a test-failure, even if they happen between/after tests, right?

Comment 1

[Daniel Holbert [:dholbert]]

Update: I worked around this in my test by figuring out how to force all of the badness to happen before the test ends. (That's ideal; it means the harness will be able to reliably attribute any test-failures to the correct test.) So: this isn't blocking me in bug 1833279 at all, at this point.)

But it's not always entirely possible to force a crashtest's fatal assertion to happen during the crashtest itself, and it would still be really good to fix the harness so that these asserts/crashes/etc. will still turn the harness orange and give us a clue that something went wrong. It's extremely weird that the test harness ends up green despite hitting an unexpected content-process crash.

Comment 2

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:jgraham, could you have a look please?

For more information, please visit BugBot documentation.

Comment 3

[James Graham [:jgraham]]

So the problem in general here is "what if we try to import an upstream change which causes a crash we can't attribute to a specific test". If we turn CI orange in that case our import is blocked until we disable the test that's responsible. That would force us to file a bug on it, but I suspect it's worse to disable the test (but have a bug on file) than to leave the crash in the output logs (but without a bug on file) in terms of the chances of someone fixing the bug.

Given this limitation, the remaining question is whether the harness could do more to attribute the failure to the test. Maybe we could try always navigating to about:blank after completing a crashtest? That would at least handle the case where we don't crash until a navigation (but of course there could still be cases where we crash arbitrarily long after the test has completed).

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to James Graham [:jgraham] from comment #3)

If we turn CI orange in that case our import is blocked until we disable the test that's responsible. That would force us to file a bug on it, but I suspect it's worse to disable the test (but have a bug on file) than to leave the crash in the output logs (but without a bug on file) in terms of the chances of someone fixing the bug.

I disagree which situation is worse/less-likely-to-result-in-a-fix here.

With the current state of things as described in comment 0: if we import a test with a delayed-crash and we handle that delayed crash by keeping the log green, then nobody's going to look at the log, and there's ~zero chance that any human will be aware that a crash even happened, let alone investigate/fix it.

More importantly, though: if someone lands patch that introduces a delayed/document-teardown crash (or regresses an old crash) that one of our WPT crashtests is lucky enough to trigger, the test harness won't be able to help us out by turning orange to warn us about that. That's the failure-case that I'm more concerned with.

whether the harness could do more to attribute the failure to the test.

Yeah, this is difficult. test-verify might be our best tool here, but in many cases we end up gated on humans-figuring-it-out based on context and trial-and-error (this is already the case for delayed crashes in our other test suites, particularly crashtests).

In our other test suites, when the crash is misattributed to the wrong test (if we have a delayed crash that fires during the next test, for example), we often end up figuring that out based on simply reasoning about the backtrace (e.g. if it contains document-teardown code, or functions related to features that are only used in some earlier test); or by checking the history-of-when-the-crash-started; or by speculatively disabling the incorrectly-blamed-test and then noticing that the same crash crops up in the next one, and working backwards from there.

Maybe we could try always navigating to about:blank

That's probably a good idea and might improve reliability here (especially if we take care to properly attribute crashes-during-this-about:blank-load to the former test rather than to about:blank :) ). It might be good enough to avoid many/most of the issues here.