973384 - Assertion failure when <track src> is a URL that redirects

Status: RESOLVED FIXED

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Did not receive all required callbacks!: 'NS_FAILED(mResult) || mExpectedCallbacks == 0', file netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp, line 69

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

This bug also causes the throbber to spin forever and makes Firefox leak nsGlobalWindow & nsDocument.

Comment 3

[Ralph Giles (:rillian) | needinfo me]

Rick, can you take a look?

Comment 4

[Rick Eyre (:reyre)]

Will do.

Comment 5

[Ralph Giles (:rillian) | needinfo me]

This blocks the tab for me, not the whole ui. I think we should wontfix 28 and patch 29 if the fix is straightforward. Redirects should be rare for track elements in the near term.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

If this isn't a release blocker for 28 we won't track, you can nominate an uplift to Aurora when ready and we can evaluate the risk/rewards along with the timing.

Comment 7

[Brad Dougherty]

We are able to work around this with some effort, but right now we (Vimeo) use redirects for our track urls, so this is kind of important for us.

I've set up a test case here: https://brad.is/reportingbugs/texttracks/redirect/ Both videos properly show the captions in Chrome, Safari, and IE.

Comment 8

[Vincent Valot]

(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #6)
> If this isn't a release blocker for 28 we won't track, you can nominate an
> uplift to Aurora when ready and we can evaluate the risk/rewards along with
> the timing.

We just met the issue as we use a load balancer to dispatch to our subtitles assets...

--

Could you continue the work on this bug ? Others browsers works well and we have to make specific cases for Firefox... which is quite a pain, and we're unable to use native HTML5 tracks.


Thanks !

Vincent.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Anthony, is anyone available to look at this?  It seems work died off on it years ago, but its affecting large sites on the web.

Comment 10

[tzi]

Here is a unit test case: 

1/ http://codepen.io/tzi/full/OWvqdr/
The VTT file is behind a 302 (cause of a load balancer)
The subtitles aren't displayed in Firefox.
No problem in blink browsers.

2/ http://codepen.io/tzi/full/EZEMzW/
The VTT file is directly loaded
This time, the subtitles are displayed in Firefox

Comment 11

[Henry Chang [:hchang]]

Attachment: Bug 973384 - Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns.

Review commit: https://reviewboard.mozilla.org/r/109364/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109364/

Comment 12

[Henry Chang [:hchang]]

Hi Ralph,

Could you please have it a review again? I am not able to carry over r+ to reviewboard.

BTW, given that we actually have web-platform test for track redirection 
(but haven't been enabled yet), do you think if we should add our own test
case at this time or we can wait for the web-platform tests enabled? (coming soon!)

Thanks.

Comment 13

[Vincent Valot]

Thanks/all for the quick reply ! 

We definitely need this :)

Comment 14

[Ralph Giles (:rillian) | needinfo me]

Comment on attachment 8833144 [details]
Bug 973384 - Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns.

https://reviewboard.mozilla.org/r/109364/#review110652

Comment 15

[Ralph Giles (:rillian) | needinfo me]

Once we have confirmation this fixes the issue cleanly we should request uplift to Aurora53, since this is an issue affecting major sites.

Comment 16

[Henry Chang [:hchang]]

Comment on attachment 8833144 [details]
Bug 973384 - Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109364/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/876e2bc7fc06
Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns. r=rillian

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/876e2bc7fc06

Comment 19

[tzi]

Thank you so much for this fix!
We really appreciate your responsiveness: it's pretty incredible to solve this 3 years bug in 5 days :D

We would confirm the solving, once ship in the Nightly.

Have a good day!
Thomas.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This looks like a good candidate for backporting once verified on Nightly?

Comment 21

[Ralph Giles (:rillian) | needinfo me]

Thomas, this fix should be available in today's Firefox Nightly. I don't see an assertion from the test case. If you could confirm the issue is fixed for you, we'd appreciate it.

Comment 22

[Vincent Valot]

Hi Ralph,

I can confirm the issue is fixed in the nightly.
Thank you !

Comment 23

[Ralph Giles (:rillian) | needinfo me]

Comment on attachment 8833144 [details]
Bug 973384 - Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns.

Nominating this for uplift to 53 and 52 to get the fix deployed more quickly.

Approval Request Comment
[Feature/Bug causing the regression]: WebVTT video subtitle playback.

[User impact if declined]: We will not honor http redirects on subtitle files, causing some files to fail to load.

[Is this code covered by automated tests?]: Yes.

[Has the fix been verified in Nightly?]: Yes.

[Needs manual test from QE? If yes, steps to reproduce]: No.

[List of other uplifts needed for the feature/fix]: None.

[Is the change risky?]: Change is not risky for Aurora. I believe risk is acceptable for Beta.

[Why is the change risky/not risky?]: This is a one-line code change, accompanied by status updates for previously failing tests. It just corrects the loading behaviour to handle redirects the way we do for other media types. I expect any regression caused by this change should be obvious, limited to the individual subtitle track, and easy to revert.

[String changes made/needed]: None.

Comment 24

[Gerry Chang [:gchang]]

Comment on attachment 8833144 [details]
Bug 973384 - Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns.

Fix an assertion failure. Aurora53+.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/620b79612ae4

Comment 26

[Julien Cristau [:jcristau] (back on August 5)]

Comment on attachment 8833144 [details]
Bug 973384 - Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns.

fix handling of redirects for WebVTT, beta52+

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4840d6a750cc

Comment 28

[Cornel Ionce [:cornel_ionce], Desktop Release QA]

Setting qe-verify- per comment 23.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/4840d6a750cc