Closed Bug 1848205 Opened 2 years ago Closed 2 years ago

Some mochitest flavors can fail silently due a second less-useful unload-event-handler invocation, if the test navigates

Categories

(Testing :: Mochitest, defect, P1)

Default
defect

Tracking

(firefox118 fixed)

RESOLVED FIXED
118 Branch
Tracking Status
firefox118 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files)

I've been poking at bug 1845221 to see what was going on and causing silent failure-count-increments-without-any-logging there, and I think I've got to essentially the root of the issue.

Here's what's going on here:
(1) If a test reloads (or navigates to its own URL) -- as test_link.html from bug 1845221 did, by clicking a link which navigated to the same document -- then we end up firing any registered unload handlers twice. I confirmed this in a trivial a11y mochitest that I wrote which registers its own unload handler.

(2) When this happens: on the second unload handler invocation, **a bunch of data is missing or cleaned up on the window object. In particular: testwin.SimpleTest._tests.length is 0 and testwin.SimpleTest.testsLength is undefined.

(3) This causes our logic for detecting test-results-logged-after-finish to be confused, since it compares and subtracts those^ two values. The subtraction produces a wrongtestlength of NaN, and so we end up skipping the for (var i = 0; i < wrongtestlength; i++) { loop, and so we skip the logging, but we still increment the failure-count via TestRunner.updateUI([{ result: false }]);

This means we end up with a failure-count of 1 but with no logging to help out with diagnosing the failure.

I just ran across a patch that landed yesterday to address this in Bug 1844310, which added a check for this and some logging -- but I think we can make the check and the logging a little more targeted and actionable. (In particular, we can check testsLength directly; and we should log about the unload handler being invoked redundantly possibly due to navigation during the test, rather than logging after finish. Since, as described above, the problem is not actually that we're logging after finish. It just happens to be that we're hitting an error scenario during the second time we go through the code that checks for logging after finish.)

So: per bug 1844310 comment 9, I think we should probably back out the patch there and land a patch that I'll be posting here shortly.

Blocks: 1845221
See Also: → 1844310
Severity: -- → S3
Priority: -- → P1

This patch doesn't affect behavior; it's just splitting one compound check into
two nested ones, and adjusting the indentation.

I'm doing this because the next patch will adjust the inner logic, and
splitting it up this way lets me do that in a more targeted way.

As described in the bug, this can sometimes happen when mochitests navigate,
e.g. due to synthesizing a click on a link to the same document.

part 3 (just added) is just to be sure we cover all our bases, in case there's another independent way of triggering this same silent failure.

I'll do some try runs with these patches and with test_link.html added back (as a canary that's expected to trip over the new check from part 2). Assuming everything looks good, we can request for bug 1844310 to be backed out and then land the stack here.

Try runs look good.

Here are several try runs, layered on top of central with bug 1844310 backed out, and then with patches as-described below:

(1) With this patch stack (green, modulo known/unrelated intermittents):
https://treeherder.mozilla.org/jobs?repo=try&revision=086ba65df462e88131020cf72ea775d99de20acb

(2) With this patch stack, plus the known-problematic test_link.html a11y mochitest added back -- good news, it does successfully trigger the new logging:
https://treeherder.mozilla.org/jobs?repo=try&revision=aaac90ed9f820f8c97c548d26ba2a655a6add607

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/actions/test_link.html fired an unload callback with missing test data, possibly due to the test navigating or reloading

(3) ...and now with the undefined-handling case disabled, to see if the "cover all our bases" part-3 fallback-handling kicks in and works properly (good news, it does):
https://treeherder.mozilla.org/jobs?repo=try&revision=20fd25285280419f69be66be8a117b613ba4899a

Duplicate of this bug: 1844310
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/df7c67c3519e part 1: Split up two-part condition in TestRunner.js. r=jmaher https://hg.mozilla.org/integration/autoland/rev/b78a2a91f0d4 part 2: Log an error when mochitests hit unload handler with missing data. r=jmaher https://hg.mozilla.org/integration/autoland/rev/5f201489618f part 3: Ensure we log something when TestRunner increments the error count in testFinished. r=jmaher
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 118 Branch
See Also: 1844310
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: