1334537 - Yahoo Videos load endlessly

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

[Affected versions]:
- latest Nightly 54.0a1 (2017-01-27)
- latest Aurora 53.0a2 (2017-01-26)
- 52.0b1 build2 (20170124094647)


[Affected platforms]:
- Windows 10 x64
- Windows 7 x64
- Ubuntu 16.04 x64	
- Mac OS X 10.11.6

[Steps to reproduce]:
1. Launch Firefox
2. Go to https://www.yahoo.com/music/ (or to https://www.yahoo.com/news/), choose a random video and play it

[Expected result]:
- The chosen video is successfully loaded 
- Audio and video play smoothly

[Actual result]:
- The chosen video loads even for several minutes and after that an error message is displayed (see https://drive.google.com/file/d/0B0nYKG6PRiCcSXRVdWpmM0xSTnM/view?usp=sharing and https://drive.google.com/file/d/0B0nYKG6PRiCcYUo1R3pPTzZ5WHc/view?usp=sharing) - this can be intermittent, a page refresh can partially help sometimes - the video loads up to a point, then audio and video lag and the endless loading starts again

[Regression range]:
- This seems to be a regression, since it is not reproducible on 51.0.1 build3 (20170125094131)
- I will provide the necessary details as soon as possible

[Additional notes]:
- Tried the same steps on Google Chrome and the issue is not reproducible
- The bug is not e10s related

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d19cffeae1febc6d669a024340de25228e544475&tochange=24d97dc514d4b8fe608e9aee7527615c1e613606

Regressed by: Bug 1300659

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]:affected to major site.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I'm investigating.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

So, I disabled the timer changes from bug 1300659 and the page still failed to load for me.  Maybe because I was in a DEBUG build changing the timing.  If this is a timing dependent race in the page then it might be up to the site to fix it on their end.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Building opt to continue investigating.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

So I couldn't fix this by backing out bug 1300659.  Also, I built the revision where bug 1300659 landed and I couldn't reproduce.

I did a new mozregression and landed on bug 1324430.  That might make sense since it seems the XHRs pulling the video data stall out.  I'm going to investigate to see if that is really the cause.

Comment 7

[Alice0775 White]

I'm sorry to confuse you

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Alice0775 White from comment #7)
> I'm sorry to confuse you

No problem.  But were you able to see the same thing?  I still don't trust that this is easily bisected.  Trying to verify, but compiling is slow here...

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Ok, on:

https://www.yahoo.com/music/

Clicking the first video about AFI  I locally get stalled video data with rev:

  f4b165800dd7

But there is no stalling on the proceeding rev:

  048ad2874f2c

This doesn't seem to affect the advertisement, only the video itself.

Andrea or Boris, do you either of you have any ideas here?  Seems like the XHRs are not acting correctly after this.  (I have not debugged the site, though.)

Comment 10

[Boris Zbarsky [:bzbarsky]]

One thing you could try is to effectively back out that changeset on tip (or beta 52).  Specifically, modify XMLHttpRequestStringBuffer::GetAsString(DOMString& aString, uint32_t aLength) as follows:

1)  s/if (buf)/if (false)/
2)  Remove the "MOZ_ASSERT(mData.IsEmpty());"

That should effectively get you back to the pre-bug 1324430 behavior, I think.  If that fixes things, we need to do some thinking about what's going on...

The other thing worth checking is whether bug 1330593 has landed on beta (which I don't think it has), and if not whether applying those patches to beta fixes this problem.  Those patches _have_ landed on m-c, though and I think they should be in the nightly build mentioned in comment 0, so it doesn't seem too likely this will help....

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #10)
> One thing you could try is to effectively back out that changeset on tip (or
> beta 52).  Specifically, modify
> XMLHttpRequestStringBuffer::GetAsString(DOMString& aString, uint32_t
> aLength) as follows:
> 
> 1)  s/if (buf)/if (false)/
> 2)  Remove the "MOZ_ASSERT(mData.IsEmpty());"
> 
> That should effectively get you back to the pre-bug 1324430 behavior, I
> think.  If that fixes things, we need to do some thinking about what's going
> on...

It seems to fix the video data stalling on my machine.  I'd love someone else to confirm, though.  I don't get exactly the same behavior as comment 0.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure to clear out our external string cache if the length doesn't match, since our length no longer needs to match our stringbuffer

Without this change, we could do a "short" get of a string (e.g. from
XMLHttpRequest), then do another get that returns the same stringbuffer but a
longer length.  But we wouldn't notice, hit our cache, and return the same,
too-short, external string.  The site would not see the new data it should have
seen.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8832266 [details] [diff] [review]
Make sure to clear out our external string cache if the length doesn't match, since our length no longer needs to match our stringbuffer

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1324430
[User impact if declined]: Some sites will not work.
[Is this code covered by automated tests?]: Yes, in the patch.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: Yes.  Please see
   comment 0.
[List of other uplifts needed for the feature/fix]:  Shouldn't need any.
[Is the change risky?]:  I don't think it is, but this is fiddly code...
[Why is the change risky/not risky?]:  Just making an existing cache not end up
   with a cache hit incorrectly.
[String changes made/needed]: None.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8832266 [details] [diff] [review]
Make sure to clear out our external string cache if the length doesn't match, since our length no longer needs to match our stringbuffer

Review of attachment 8832266 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/xpcpublic.h
@@ +241,2 @@
>          void* mBuffer;
> +        uint32_t mLength;

Do we avoid initializing these members to known values in a default constructor as an optimization?  It would be nice to initialize them if possible...

Comment 15

[Boris Zbarsky [:bzbarsky]]

We could do that; the init would happen once per zone, so wouldn't be a big deal perf wise.  We mostly didn't do it because it didn't matter, so we didn't think about it: we allocate the cache and then immediately write into it.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 8832266 [details] [diff] [review]
Make sure to clear out our external string cache if the length doesn't match, since our length no longer needs to match our stringbuffer

Review of attachment 8832266 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this.  r=me with or without the below changes.

WRT bkelly's default initialization suggestion, I think compilers would optimize out the dead stores.  I'm +/- on the idea, though.

::: js/xpconnect/src/XPCString.cpp
@@ +47,5 @@
>  
>      ZoneStringCache* cache = static_cast<ZoneStringCache*>(JS_GetZoneUserData(zone));
>      if (cache) {
>          cache->mBuffer = nullptr;
> +        // No need to change cache->mLength; no one will look at it anyway.

I would feel epsilon better if we zeroed mLength here...

@@ +72,5 @@
>      // when flattening an external string.
>      ZoneStringCache* cache = static_cast<ZoneStringCache*>(JS_GetZoneUserData(zone));
>      if (cache && cache->mBuffer == buf) {
>          cache->mBuffer = nullptr;
> +        // No need to change cache->mLength; no one will look at it anyway.

...and here, just so all the data inside the cache is some kind of consistent.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0c4afeb0e3
Make sure to clear out our external string cache if the length doesn't match, since our length no longer needs to match our stringbuffer.  r=froydnj

Comment 18

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

Tracked for 52+

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd0c4afeb0e3

Comment 20

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

Comment on attachment 8832266 [details] [diff] [review]
Make sure to clear out our external string cache if the length doesn't match, since our length no longer needs to match our stringbuffer

Fix for video playback, let's take this on aurora and beta. It should make it into next week's beta 4 build.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7836b720028c

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b9e7643b8be6

Comment 23

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  54.0a1 (2017.01.27) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 52.0b4, 53.0a2, 54.0a1 on Win 8.1 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.