Yahoo Videos load endlessly

VERIFIED FIXED in Firefox 52

Status

()

defect
VERIFIED FIXED
2 years ago
a month ago

People

(Reporter: JuliaC, Assigned: bzbarsky)

Tracking

({regression})

Trunk
mozilla54
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 unaffected, firefox52+ verified, firefox53 verified, firefox54 verified)

Details

(URL)

Attachments

(1 attachment)

[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 hidden (obsolete)

Comment 2

2 years ago
[Tracking Requested - why for this release]:affected to major site.
I'm investigating.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
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.
Building opt to continue investigating.
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

2 years ago
I'm sorry to confuse you
Blocks: 1324430
No longer blocks: 1300659
(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...
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.)
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(bzbarsky)
Flags: needinfo?(amarchesini)
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....
Flags: needinfo?(bzbarsky)
(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.
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.
Attachment #8832266 - Flags: review?(nfroyd)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8832266 - Flags: approval-mozilla-beta?
Attachment #8832266 - Flags: approval-mozilla-aurora?
Flags: needinfo?(amarchesini)
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...
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 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.
Attachment #8832266 - Flags: review?(nfroyd) → review+

Comment 17

2 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
Tracked for 52+

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bd0c4afeb0e3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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.
Attachment #8832266 - Flags: approval-mozilla-beta?
Attachment #8832266 - Flags: approval-mozilla-beta+
Attachment #8832266 - Flags: approval-mozilla-aurora?
Attachment #8832266 - Flags: approval-mozilla-aurora+
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.