554524 - improve nsNPAPIPluginInstance's stream management

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

Attachment: fix v1.0

We should improve nsNPAPIPluginInstance's stream management. Top two priorities here:

1. Streams shouldn't keep a strong reference to their instance. This puts us at risk for cyclical references and makes incorrect stream handling harder to debug.

2. Stop using an nsInstanceStream-based linked list to keep track of streams. Just use an nsTArray of nsRefPtr objects.

Comment 1

[Johnny Stenback (:jst)]

Comment on attachment 434423 [details] [diff] [review]
fix v1.0

r=jst (but bsmedberg, feel free to look as well if you want to, of course).

Comment 2

[Josh Aas]

Attachment: fix v1.1

update for current mozilla-central

Comment 3

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/10d2046d2b64

Comment 4

[Josh Aas]

backed out due to bug 558130

http://hg.mozilla.org/mozilla-central/rev/65aca11d2b37

Comment 5

[Josh Aas]

Attachment: fix v2.0

Fixes the crash that led to a backout of v1.1.

Comment 6

[Josh Aas]

Attachment: fix v2.1

Fix some test failures.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 449011 [details] [diff] [review]
fix v2.1

- In nsNPAPIPluginStreamListener::CleanUpStream(NPReason reason):

 {
+  // Null out the instance (mInst) here, we can't depend on it existing after this method completes.
+
   nsresult rv = NS_ERROR_FAILURE;
 
-  if (mStreamCleanedUp)
+  if (mStreamCleanedUp || !mInst)
     return NS_OK;
 
   mStreamCleanedUp = PR_TRUE;
 
   StopDataPump();
 
   // Seekable streams have an extra addref when they are created which must
   // be matched here.
   if (NP_SEEK == mStreamType)
     NS_RELEASE_THIS();

Seems like if this is a seekable stream and mInst got nulled out already, we'll never release this extra reference to the listener and then leak it. Or did I miss another release of this extra addref?

Comment 8

[Josh Aas]

That comment shouldn't be there, it is from an older version of the patch that did have the problem you described. The current patch does not actually null out mInst until all the code in "CleanUpStream" has executed.

Comment 9

[Josh Aas]

Attachment: fix v2.2

Update comment.

Comment 10

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/8e2ff18bc67e

Comment 11

[Josh Aas]

Backed out because I discovered a bad crash when testing other patches.

http://hg.mozilla.org/mozilla-central/rev/9c15f02468dc
http://hg.mozilla.org/mozilla-central/rev/6d425b6517d6

1. have my patch applied and use the latest Flash 10.1 rc (rc7, iirc)
2. start playing a YouTube video that has sound in a window with a single tab
3. close the top-level window using cmd-w

This will crash parent and child processes, but before they actually go down the sound from the video will continue to play with no window open.

Comment 12

[Josh Aas]

Attachment: fix v3.0

This patch fixes the crash on destroy with ABP. It is a less aggressive version of fix v2.x in that it doesn't change the actual ownership model, it just improves the object/memory management code.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 451110 [details] [diff] [review]
fix v3.0

-  nsInstanceStream *mStreams;
+  nsTArray<nsNPAPIPluginStreamListener*> mStreams;

It occurred to me that we should probably call this mStreamListeners instead of mStreams, since it really is an array of stream listeners. But I'm fine with doing that as part of a followup cleanup bug if you don't want to do that rename here.

r=jst

Comment 14

[Josh Aas]

Attachment: fix v3.1

mStreams -> mStreamListeners

Comment 15

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/51136ef1b3e1