1194600 - Many Chinese live streaming websites don't work on Firefox 40 with Asynchronous Plugin Initialization

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Dapeng Wei]

UA:Mozilla/5.0 (Windows NT 10.0; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0

Many Chinese live streaming websites don't work on Firefox 40 with Asynchronous Plugin Initialization.

Sites list:
http://live.bilibili.com/ 
Alexa Global Rank:306 
Rank in China:60   

http://www.douyutv.com/
Alexa Global Rank:679
Rank in China:97

Steps to reproduce:
1.Visit these websites on Firefox 40 with default profile.
2.The Flash player on websites loaded, but not load the streaming video.
3.Set dom.ipc.plugins.asyncInit false. Restart Firefox.
4.Visit the websites again, that works well.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Thanks for the report. I'll take a look.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch

Some streams are failing because the buffer is not being grown to accommodate additional incoming data. The buffer growth was predicated upon the stream being suspended. If the stream was not suspended, the buffer would not be able to hold new data and nothing would be read from the input stream, eventually triggering an error in Necko.

I don't see any reason why the mIsSuspended predicate needs to be there, as the buffer needs to be large enough regardless.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch

Updated comments around affected code.

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 8652615 [details] [diff] [review]
Patch

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

::: dom/plugins/base/nsNPAPIPluginStreamListener.cpp
@@ +560,4 @@
>        uint32_t amountRead = 0;
>        rv = input->Read(mStreamBuffer + mStreamBufferByteCount, bytesToRead,
>                         &amountRead);
>        NS_ENSURE_SUCCESS(rv, rv);

looks like we would fall through to here and fail this read call due to lack of buffer? now we go ahead and reallocation to collect the data. This is ok  for a suspended stream?

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 8652615 [details] [diff] [review]
Patch

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

::: dom/plugins/base/nsNPAPIPluginStreamListener.cpp
@@ +548,4 @@
>          mStreamBufferSize = mStreamBufferByteCount + length;
>          char *buf = (char*)PR_Realloc(mStreamBuffer, mStreamBufferSize);
>          if (!buf)
>            return NS_ERROR_OUT_OF_MEMORY;

if this reallocation fails, mStreamBufferSize will not match mStreamBuffer. Is that ok?
I realize this is a prior issue

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #4)
> Comment on attachment 8652615 [details] [diff] [review]
> Patch
> 
> Review of attachment 8652615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/base/nsNPAPIPluginStreamListener.cpp
> @@ +560,4 @@
> >        uint32_t amountRead = 0;
> >        rv = input->Read(mStreamBuffer + mStreamBufferByteCount, bytesToRead,
> >                         &amountRead);
> >        NS_ENSURE_SUCCESS(rv, rv);
> 
> looks like we would fall through to here and fail this read call due to lack
> of buffer? now we go ahead and reallocation to collect the data. This is ok 
> for a suspended stream?

My understanding is that this is exactly what we want. While suspended, Necko still expects us to take the data as part of the API contract, so we need to read it into the buffer to hold it until the stream is resumed.

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #5)
> Comment on attachment 8652615 [details] [diff] [review]
> Patch
> 
> Review of attachment 8652615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/base/nsNPAPIPluginStreamListener.cpp
> @@ +548,4 @@
> >          mStreamBufferSize = mStreamBufferByteCount + length;
> >          char *buf = (char*)PR_Realloc(mStreamBuffer, mStreamBufferSize);
> >          if (!buf)
> >            return NS_ERROR_OUT_OF_MEMORY;
> 
> if this reallocation fails, mStreamBufferSize will not match mStreamBuffer.
> Is that ok?
> I realize this is a prior issue

Stepping through this function's callers yesterday I saw very aggressive return code checking. Everything should shut down when that error code is returned.

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/484242b6953c

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8652615 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: NPAPI plugin streams
[User impact if declined]: Gecko can't properly fulfill some network requests made by plugins. Seen more frequently with async init, but present without it too.
[Describe test coverage new/current, TreeHerder]: Plugin test suite. Also tested against links reported in this bug as well as other bugs blocking bug 1195607.
[Risks and why]: Low. Change effectively removes one predicate from an if statement which is quite trivial.
[String/UUID change made/needed]: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/484242b6953c

Comment 10

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

Comment on attachment 8652615 [details] [diff] [review]
Patch

Patch seems low risk, and the sooner we uplift to Beta we can get confirmation from our end-users that this works as expected. Aurora42+, Beta41+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c6dd528dba68

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/162d4504f6a8

Comment 27

[Petruta Horea [:phorea], Desktop QA]

I tried to reproduce with Firefox 40RC, the first link works fine and for the second one there are some streams that don't start playing (Stream not found is displayed in the web console).

Using Firefox 41 beta 5, both links are playing under Win 7 64-bit and Mac OS X 10.9.5 (here live.bilibili.com needed to be refreshed several times in order to be loaded).

Can you please use Firefox 41 beta 5 [1] and let us know if both links are correctly played now? Thank you! 
[1] https://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/41.0b5-candidates/build1/

Comment 28

[Yanfang Liu]

Verified on Win7, Win10 and Mac OSX with Firefox 41 beta 5, both links are playing well. 

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:41.0) Gecko/20100101 Firefox/41.0

BuildID: 20150827142634