Videos are not displayed after reloading a tab

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: hani.yacoub, Assigned: freesamael)

Tracking

({regression})

55 Branch
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57+ verified, firefox58+ verified)

Details

Attachments

(2 attachments)

Posted video reload tab.mp4
[Affected versions]: 
Nightly 57.0a1

[Affected platforms]:
Platforms: Windows 10 x 64, Mac OS X 10.12 and Ubuntu 16.04 x64.

[Steps to reproduce]:
1. Open Firefox.
2. Go to https://www.tutorialrepublic.com/codelab.php?topic=html5&file=video-element
3. Click on "Reload current tab".

[Expected result]:
The video is displayed.

[Actual result]:
The video is not displayed.
Thanks for reporting. 

I think this should not be video controls issue as I could not find any <video> inside the right panel after reloaded, if no <video> present then no <videocontrols> will be loaded. If we click on "Show Output", content shows up again, so I would suspect maybe the client script failed to reload the preview panel after reloaded or our somewhere else in our back-end.

Eric, do you think this is a WebCompat issue? thanks.
Flags: needinfo?(etsai)
For me, no. It works for me using Firefox 55 and Chrome. But I'm not sure the root cause, there is no error messages in web console.
But as the comment above, we can try some directions, the difference between re-enter the address and reload tab may cause different result in this page.
Flags: needinfo?(etsai)
Redirect to doc navigation according to Eric's comment that the behavior varies between "address re-entry" and click on "reload"
Component: Video/Audio Controls → Document Navigation
Product: Toolkit → Core
I can't reproduce this. Is this a recent regression?
[Tracking Requested - why for this release]: Video dom element is missing after reload page.


I can reproduce the problem on Nighty58.0a1 Windows10.

Regression window:https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0a864923a1b47b2959a4ea949f14a8e7cb396cd1&tochange=8a5a636429ea000a738efd1d9189c2a5b696de8d

Regressed by: Bug 1375833

@:freesamael,

It seems your patch causes the problem. Could you look at this?
Blocks: 1375833
Flags: needinfo?(sawang)
Sadly to see a regression, but I guess that's the fun part working on session history.
Assignee: nobody → sawang
Flags: needinfo?(sawang)
Haven't figured out why but I noticed it hits this warning on reloading the wyciwyg:// frame entry:

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/parser/html/nsHtml5StreamParser.cpp#1007

I believe this is not reloading-specific; it should also happen in general history navigation, so I tried that if I test it in this way:

1. Set the pref "browser.sessionhistory.max_total_viewers" to 0 (essentially disable bfcache)
2. Navigate to https://www.tutorialrepublic.com/codelab.php?topic=html5&file=video-element
3. Navigate to https://www.mozilla.com
4. Press back

Then I can reproduce the symptom on at least as old as Firefox 52 ESR on macOS.
(The warning doesn't seem to be important)
Another interesting thing is that the video element does show up for a very short time, and then the whole document becomes empty (even the <title> tag disappeared from inspector).
So this is what happened on reloading:

1. The iframe's nsDocShell::LoadHistoryEntry created a WYCIWYG channel, and asyncOpen'd it.
2. The root document received DOMContentLoaded, and it tried to execute the following script to update the iframe:

    function updatePreview(){
        var previewFrame = document.getElementById('preview');
        var preview =  previewFrame.contentDocument ||  previewFrame.contentWindow.document;
        preview.open();
        preview.write(editor.getValue());
        preview.close();
    }

3. The `document.open` above caused nsDocShell::Stop, which canceled the WYCIWYG channel.
4. The consequent `document.write` / `document.close` composed the document.
5. Although the WYCIWYG channel had been canceled, nsDocShell has asyncOpen'd it, and nsIChanned's contract is:
   "If asyncOpen returns successfully, the channel promises to call at least onStartRequest and onStopRequest."
   It invoked onStartRequest after the document generated by document.write has been composed.
6. nsDocumentOpenInfo::OnStartRequest caused nsDocShell::CreateContentViewer, which setup a new blank viewer.
   But the channel has been canceled. There wouldn't be any onDataAvailable. The viewer became and kept blank.

It's quite interesting, but I don't have any proposal yet. I'm not sure how to deal with this properly. Suggestions are welcome.
So what caused the regression? Why things were working before bug 1375833?
(In reply to Olli Pettay [:smaug] from comment #11)
> So what caused the regression? Why things were working before bug 1375833?

We dropped even static iframe history entries on reloading before, so it wouldn't enter this code path. As I mentioned in comment 7, if I intentionally disable bfcache, then this symptom can happen on history navigation (goback) on Firefox 52, and maybe older.
> nsDocumentOpenInfo::OnStartRequest caused nsDocShell::CreateContentViewer

Why is that happening?

I would expect that in nsDocumentOpenInfo::OnStartRequest we hit this code (some error-handling elided):

  nsresult status;
  rv = request->GetStatus(&status);
  if (NS_FAILED(status)) {
    LOG_ERROR(("  Request failed, status: 0x%08" PRIX32, static_cast<uint32_t>(rv)));
    //
    // The transaction has already reported an error - so it will be torn
    // down. Therefore, it is not necessary to return an error code...
    //
    return NS_OK;
  }

And never talk to the docshell at all.  What does "status" end up being in this code?
Flags: needinfo?(sawang)
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #13)
> What does "status" end up being in this code?

Thanks for the hint, Boris! The status turns out to be the most important thing but I missed!

The case I hit was that on parent side, WyciwygChannelParent::SendOnStartRequest happened before RecvCancel, so the status code sent to the child was NS_OK. Comparing to HttpChannelChild [1], it seems all I need to do is not to apply statusCode sent from the parent in WyciwygChannelChild::OnStartRequest if mCanceled. Then the statusCode will remain in NS_BINDING_ABORTED. Thanks!

[1] http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/netwerk/protocol/http/HttpChannelChild.cpp#555-557
Flags: needinfo?(sawang)
Glad to help.  :)
Comment on attachment 8913055 [details]
Bug 1401522 - Don't apply statusCode sent from parent if mCanceled or mStatus has been modified in the child.

https://reviewboard.mozilla.org/r/184448/#review189800

::: netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp:194
(Diff revision 2)
>  {
>    LOG(("WyciwygChannelChild::RecvOnStartRequest [this=%p]\n", this));
>  
>    mState = WCC_ONSTART;
>  
> +  if (!mCanceled) {

Shouldn't you check && NS_SUCCEEDED(mStatus) too?  Right now it looks like mStatus can only be failure if mCanceled is true, but checking both would be more robust against other things that may end up failing the child-side channel...
Comment on attachment 8913055 [details]
Bug 1401522 - Don't apply statusCode sent from parent if mCanceled or mStatus has been modified in the child.

https://reviewboard.mozilla.org/r/184448/#review189800

> Shouldn't you check && NS_SUCCEEDED(mStatus) too?  Right now it looks like mStatus can only be failure if mCanceled is true, but checking both would be more robust against other things that may end up failing the child-side channel...

You're right. I didn't understand what that was for when looking at HttpChannelChild yesterday.
Tracking 57/58 since this is a regression and we should show the content after a reload.
Comment on attachment 8913055 [details]
Bug 1401522 - Don't apply statusCode sent from parent if mCanceled or mStatus has been modified in the child.

https://reviewboard.mozilla.org/r/184448/#review190184

Yes, this is it.  Thanks!
Attachment #8913055 - Flags: review?(honzab.moz) → review+
This bug tracks 57 and we have a patch. Should we land this and uplift?
Flags: needinfo?(sawang)
Andrew, or Honza, can you help us get this landed and nominated for uplift quickly? 
Looks like Samael is on PTO.
Flags: needinfo?(overholt)
Flags: needinfo?(honzab.moz)
Yep.
Flags: needinfo?(sawang)
Flags: needinfo?(overholt)
Flags: needinfo?(honzab.moz)
I assume it's not checked-in yet.
Keywords: checkin-needed
Comment on attachment 8913055 [details]
Bug 1401522 - Don't apply statusCode sent from parent if mCanceled or mStatus has been modified in the child.

Can I use the same patch to uplift? WyciwygChannelChild.cpp hasn't been changed recently, the patch should be able to apply on 57.

Approval Request Comment
[Feature/Bug causing the regression]: The bug has been there since we made WyciwygChannel e10s, but the recent change in Bug 1375833 causes the symptom being observable on reloading.
[User impact if declined]: User may see a blank page on history navigation or reloading in certain circumstance.
[Is this code covered by automated tests?]: We don't have a test case specifically for this bug.
[Has the fix been verified in Nightly?]: No it's not landed on Nightly channel yet.
[Needs manual test from QE? If yes, steps to reproduce]: Apply the STR in comment 0.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It adds a simple check in WyciwygChannelChild to ensure failed status set by child is kept on OnStartRequest. The same check has been used in HttpChannelChild and FTPChannelChild for a few years.
[String changes made/needed]: No
Attachment #8913055 - Flags: approval-mozilla-beta?
Priority: -- → P1
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8ebfa90c4ebc
Don't apply statusCode sent from parent if mCanceled or mStatus has been modified in the child. r=mayhemer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ebfa90c4ebc
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment on attachment 8913055 [details]
Bug 1401522 - Don't apply statusCode sent from parent if mCanceled or mStatus has been modified in the child.

Fixes a recent regression, beta57+
Attachment #8913055 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
I managed to reproduce the bug using an older version of Nightly (2017-09-21) on Windows 10x64, Ubuntu 16.04x64 and Mac OS X 10.11. 
I retested everything using the latest Nightly and beta 57.0b8 on the same platforms, but the bug is not reproducing anymore. Every time I reloaded the tab (using the reload button from the toolbar, using CTRL+R / Cmd+R and right-click on tab -> Reload Tab) the video was displayed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.