1401522 - Videos are not displayed after reloading a tab

Status: VERIFIED FIXED

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

Description

[Hani Yacoub, Desktop QA]

Attachment: 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.

Comment 1

[Ray Lin[:ralin]]

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.

Comment 2

[Eric Tsai (no more review request accepted)]

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.

Comment 3

[Ray Lin[:ralin]]

Redirect to doc navigation according to Eric's comment that the behavior varies between "address re-entry" and click on "reload"

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I can't reproduce this. Is this a recent regression?

Comment 5

[Alice0775 White]

[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?

Comment 6

[Samael Wang [:freesamael] (away for now)]

Sadly to see a regression, but I guess that's the fun part working on session history.

Comment 7

[Samael Wang [:freesamael] (away for now)]

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.

Comment 8

[Samael Wang [:freesamael] (away for now)]

(The warning doesn't seem to be important)

Comment 9

[Samael Wang [:freesamael] (away for now)]

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).

Comment 10

[Samael Wang [:freesamael] (away for now)]

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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

So what caused the regression? Why things were working before bug 1375833?

Comment 12

[Samael Wang [:freesamael] (away for now)]

(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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 14

[Samael Wang [:freesamael] (away for now)]

(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

Comment 15

[Boris Zbarsky [:bzbarsky]]

Glad to help.  :)

Comment 16

[Samael Wang [:freesamael] (away for now)]

Attachment: Bug 1401522 - Don't apply statusCode sent from parent if mCanceled or mStatus has been modified in the child.

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

Comment 17

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8913055 [details]
Bug 1401522 - Don't apply statusCode sent from parent if mCanceled or mStatus has been modified in the child.

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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 19

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8913055 [details]
Bug 1401522 - Don't apply statusCode sent from parent if mCanceled or mStatus has been modified in the child.

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

Comment 20

[Samael Wang [:freesamael] (away for now)]

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.

Comment 21

[Marcia Knous [:marcia]]

Tracking 57/58 since this is a regression and we should show the content after a reload.

Comment 22

[Honza Bambas (:mayhemer)]

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!

Comment 23

[Jim Mathies [:jimm]]

This bug tracks 57 and we have a patch. Should we land this and uplift?

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrew, or Honza, can you help us get this landed and nominated for uplift quickly? 
Looks like Samael is on PTO.

Comment 25

[Andrew Overholt [:overholt]]

Yep.

Comment 26

[Samael Wang [:freesamael] (away for now)]

I assume it's not checked-in yet.

Comment 27

[Samael Wang [:freesamael] (away for now)]

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
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

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8ebfa90c4ebc

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/47c55e4e8ec9

Comment 32

[Oana Botisan, Desktop QA]

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.