1751790 - Failing WPT test verify pass (frequent timeouts)

Status: RESOLVED FIXED

(Core :: DOM: Streams, defect, P1)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: test-verify.log

Running ./mach wpt --headless testing/web-platform/tests/streams/ --verify we fail for having unstable results. (This is after I updated the test expectations to match what DOM streams ought to have).

It appears we've got some tests that are timing out 3/10 times.

Attached is the log.

I'm attempting to get more info now.

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

A pernosco trace of one of the tests timing out (under rr a 3/10 timeout became 5/10)

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Thanks to a suggestion from Olli, I managed to narrow down to at leastt one source of timeout.

Essentially, if we put

test(() => {

  const sizeFunction = (new ByteLengthQueuingStrategy({ highWaterMark: 5 })).size;

}, 'ByteLengthQueuingStrategy: size behaves as expected with strange arguments');

as the only test in testing/web-platform/tests/streams/queuing-strategies.any.js, and run ./mach wpt --headless testing/web-platform/tests/streams/queuing-strategies.any.js --verify it will fail with a timeout. Weirdly, the largely identically implemented CountQueuingStrategy doesn't share the same problem.

Comment 3

[Tom S. (please needinfo tschuster)]

You forgot to trace mByteLengthQueuingStrategySizeFunction in nsIGlobalObject.

Comment 4

[Tom S. (please needinfo tschuster)]

https://phabricator.services.mozilla.com/D134084#inline-746182

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1751790 - Add missing cycle collection for ByteLengthQueuingStrategy size function r?saschanaz

Thanks to evilpie for spotting the omission!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ea481dd8166a
Add missing cycle collection for ByteLengthQueuingStrategy size function r=saschanaz

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

The landed path is necessary, but not sufficient to get us to a clean --verify run. I'm still seeing timeouts. Setting leave-open.

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

Kagami points out this reduced version of idlharness.any fails when run in worker context. Same issue as before, 3/7 timeouts.

// META: global=window,worker
// META: script=/resources/WebIDLParser.js
// META: script=/resources/idlharness.js
// META: timeout=long

idl_test(
  ['streams'],
  ['dom'], // for AbortSignal
  async idl_array => {
    idl_array.add_objects({
      ByteLengthQueuingStrategy: ["new ByteLengthQueuingStrategy({ highWaterMark: 5 })"],
    });
  }
);

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

Attachment: testcase.zip

I'm not sure why, but somehow importScripts and ByteLengthQueuingStrategy hate each other.

Comment 10

[Tom S. (please needinfo tschuster)]

Unlink support also needs to be added: https://hg.mozilla.org/integration/autoland/file/ea481dd8166a2d70e8a0fcd40a3ff308b0341243/dom/base/nsIGlobalObject.cpp#l138. But if this also affects CountQueueingStrategy then I am not sure what is going wrong. Maybe there is some other place we don't traverse/unlink properly? Or the worker code doesn't call it?

Comment 11

[Tom S. (please needinfo tschuster)]

When I run the test from comment 8 I just get crashes with MOZ_CRASH(Found dangling CheckedUnsafePtr). This seems to have been introduced yesterday by bug 1744025.

Comment 12

[Tom S. (please needinfo tschuster)]

Attachment: Bug 1751790 - Add missing unlinking for ByteLengthQueuingStrategy size function r?saschanaz

Comment 13

[Kagami Rosylight [:saschanaz] (they/them)]

Let's track it in bug 1752075, as it seems to have different cause (and the error is different anyway).

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f39627f71c47
Add missing unlinking for ByteLengthQueuingStrategy size function r=saschanaz

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ea481dd8166a
https://hg.mozilla.org/mozilla-central/rev/f39627f71c47

Comment 16

[Matthew Gaudet (he/him) [:mgaudet]]

Ok, I don't think we'll be landing other patches under this bug,so resolving as fixed