Closed Bug 1524154 Opened 8 months ago Closed 6 months ago

Re-enable flow control for wpt that bug 1280629 disabled

Categories

(Core :: Networking, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox67 --- fixed
firefox68 --- fixed

People

(Reporter: emk, Assigned: junior)

References

()

Details

(Whiteboard: [necko-triaged])

Attachments

(2 files, 1 obsolete file)

Flow control should not have been disabled for those tests in the first place.

Honestly, I don't understand why bug 1280629 has been landed with all failing tests disabled. It defeated the sole purpose of automated tests.

Honestly, I don't understand why bug 1280629 has been landed with all failing tests disabled. It defeated the sole purpose of automated tests.

See bug 1280629 comment 51
I believe it should be under Testing/web-platform-tests

Junior, I think this needs some attention. It doesn't seem right to just disable test because of flow control in necko.

If there were a resume problem, then it was a serious regressing bug we caused.

P2 if flow control is now disabled on Release. This may even be P1 if it's enabled by default on Release.

Assignee: nobody → juhsu
Priority: -- → P2
Whiteboard: [necko-triaged]

(In reply to Junior [:junior] from comment #5)

too many noises, rebase and try again
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e2ef3787e929297500c4e510f9ebd2c8594af6e

Note for myself: need to take care of
/html/semantics/embedded-content/the-video-element/video_initially_paused.html
/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-move-to-other-document.html

It's related to pause a <video>.
Theory: The nsHttpChannel seems failing to cancel or close after suspend (pause), and isn't removed from the loadgroup. MediaStream initiates another nsHttpChannel for range request afterward.

It's easier to reproduce in mac but not in linux.

P1 as the flow control feature is now enabled in Release and apparently we shipped a bug with it, not necessarily in Necko, but somewhere.

Priority: P2 → P1

findings:
(a) when it fails, the console showes NotAllowedError: The play method is not allowed by the user agent or the platform in the current context, possibly because the user denied permission.

(b) ./mach wpt can't reproduce. firefox-release can't reproduce. moz-regression can't reproduce
Firefox nightly (or self debug build) with local served wpt can reproduce.
And it's not clearly tangled with pref network.http.send_window_size

Enable those tests already pass in the treehereder

pref media.autoplay.default is related to /html/semantics/embedded-content/media-elements/playing-the-media-resource/*

Those tests are with v.play() and v.onplaying, and the latter one is not called with e10s-bp.

Theory:
If we block autoplay, suspend will send at some time by media stream
(a) without e10s-bp, enough data could be sent before the media suspension. Hence, v.onplaying could be called.
(b) with e10s-bp, no data more than 1MB would be sent if the listener didn't handle.

Let's see treehereder for /html/semantics/embedded-content/the-video-element/video_initially_paused.html
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce146413b7e62105cfbef264c8271e4efef0dd09

Attachment #9045444 - Attachment is obsolete: true
Keywords: leave-open

Hello Alastor,
We thought some autoplay problem in web-platform test are exposed by e10s-backpressure flow control.
e10s-backpressure flow control will suspend the underlying HTTP if the listener in child process doesn't handle OnDataAvailable on time.
Therefore, only 1/4 MB is sent before the HTTP channel is suspended.

Please see comment 12.
v.onplaying should not be triggered if the v.play is blocked. But these tests are still rely on the unreliable onplaying:

/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-move-to-other-document.html
/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-remove-from-document.html
/html/semantics/embedded-content/media-elements/playing-the-media-resource/pause-move-within-document.html
/html/semantics/embedded-content/media-elements/video_008.htm

After we flip the media.autoplay.default to zero, those tests are fixed in local test and the intermittent rate goes down.

For this one,
/html/semantics/embedded-content/the-video-element/video_initially_paused.html

We try to allow the autoplay by setting pref media.autoplay.default to zero.
No timeout happen. (EXPECTED-FAIL is fixed in some platform, not sure why)

To me, I'd rather disable those tests with unreliable setup. What do you think?

Flags: needinfo?(alwu)
Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/52494bc29687
Re-enable flow control for some wpt that bug 1280629 disabled r=kershaw

Hi, Chris,
Do we have a pref list just for wpt test (like what we have for mochitest)? If not, it's possible to disable blocking autoplay for wpt by default?
Thank you.

Flags: needinfo?(alwu) → needinfo?(cpearce)

(In reply to Alastor Wu [:alwu] from comment #15)

Hi, Chris,
Do we have a pref list just for wpt test (like what we have for mochitest)? If not, it's possible to disable blocking autoplay for wpt by default?
Thank you.

Maybe:
https://searchfox.org/mozilla-central/source/testing/profiles/web-platform/user.js

... but I'd have though that

https://searchfox.org/mozilla-central/source/testing/profiles/common/user.js#54

.. would be included in the WPT user.js, and so we'd already have autoplay unblocked for WPT?

Flags: needinfo?(cpearce)

Hmm, I tested it on my local environment, it seems that we didn't block autoplay for wpt. Are you sure that we're blocking autoplay for those test you mentioned?

Flags: needinfo?(juhsu)

(In reply to Alastor Wu [:alwu] from comment #18)

Hmm, I tested it on my local environment, it seems that we didn't block autoplay for wpt. Are you sure that we're blocking autoplay for those test you mentioned?

I can reproduce it with local served wpt. (i.e., clone wpt, and host a local wpt server) However, it would be a dup of bug 1539766. Stay tuned.

Flags: needinfo?(juhsu)

Seems like a dup of bug 1539766, let's enable wpts once we land it.
https://hg.mozilla.org/try/rev/fe819d5d1dbee45c8a1d25f01fdfefecc0d936ee

Depends on: 1539766

bug 1539766 landed. treeherder looks good. Let's give it a try.

Pushed by juhsu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/799599db47f1
Re-enable flow control for media wpt r=kershaw
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Whiteboard: [necko-triaged] → [necko-triaged][checkin-needed-beta]
Whiteboard: [necko-triaged][checkin-needed-beta] → [necko-triaged]
You need to log in before you can comment on or make changes to this bug.