Iulia Cristescu, QA [:JuliaC] (away, please needinfo? cornel.ionce@softvision.ro) Reporter
	Description • 8 years ago

[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

	Alice0775 White
	Comment 2 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 3 • 8 years ago

I'm investigating.

	Ben Kelly [:bkelly, not reviewing]
	Comment 4 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 5 • 8 years ago

Building opt to continue investigating.

	Ben Kelly [:bkelly, not reviewing]
	Comment 6 • 8 years ago

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.

	Alice0775 White
	Comment 7 • 8 years ago

I'm sorry to confuse you

	Ben Kelly [:bkelly, not reviewing]
	Comment 8 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 9 • 8 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 11 • 8 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 8 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 14 • 8 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 8 years ago

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.

	Nathan Froyd [:froydnj]
	Comment 16 • 8 years ago

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.

	Pulsebot
	Comment 17 • 8 years ago

Pushed by bzbarsky@mozilla.com:
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

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 18 • 8 years ago

Tracked for 52+

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 20 • 8 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 8 years ago
bugherder uplift

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

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 8 years ago
bugherder uplift

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

	Timea Zsoldos [:zstimi/tzsoldos], Desktop QA
	Comment 23 • 8 years ago

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.