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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jruderman, Assigned: hchang)

Tracking

(Blocks 1 bug, {assertion, memory-leak, testcase})

Trunk
mozilla54
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox27 unaffected, firefox28- wontfix, firefox29 wontfix, firefox30 wontfix, firefox51 wontfix, firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

Attachments

(3 attachments)

Posted file testcase
###!!! ASSERTION: Did not receive all required callbacks!: 'NS_FAILED(mResult) || mExpectedCallbacks == 0', file netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp, line 69
Posted file stack
This bug also causes the throbber to spin forever and makes Firefox leak nsGlobalWindow & nsDocument.
Keywords: mlk
Rick, can you take a look?
Assignee: nobody → rick.eyre
Will do.
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.
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 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.
Component: Audio/Video → Audio/Video: Playback
(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.
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.
Flags: needinfo?(ajones)
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
Attachment #8833144 - Flags: review?(giles)
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.
Assignee: rick.eyre → hchang
See Also: → 1333109
Thanks/all for the quick reply ! 

We definitely need this :)
Comment on attachment 8833144 [details]
Bug 973384 - Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns.

https://reviewboard.mozilla.org/r/109364/#review110652
Attachment #8833144 - Flags: review?(giles) → review+
Once we have confirmation this fixes the issue cleanly we should request uplift to Aurora53, since this is an issue affecting major sites.
Flags: needinfo?(ajones)
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/876e2bc7fc06
Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns. r=rillian
https://hg.mozilla.org/mozilla-central/rev/876e2bc7fc06
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
This looks like a good candidate for backporting once verified on Nightly?
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.
Hi Ralph,

I can confirm the issue is fixed in the nightly.
Thank you !
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.
Flags: needinfo?(giles)
Attachment #8833144 - Flags: approval-mozilla-beta?
Attachment #8833144 - Flags: approval-mozilla-aurora?
Comment on attachment 8833144 [details]
Bug 973384 - Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns.

Fix an assertion failure. Aurora53+.
Attachment #8833144 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8833144 [details]
Bug 973384 - Call |cb| before WebVTTListener::AsyncOnChannelRedirect returns.

fix handling of redirects for WebVTT, beta52+
Attachment #8833144 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting qe-verify- per comment 23.
Flags: qe-verify-
See Also: → 1337242
You need to log in before you can comment on or make changes to this bug.