729673 - allow for plugin instance src stream load retry

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

We have a bug in which plugin src streams get marked delivered ever though they were aborted, which prevents us from try again to deliver the stream at a later time.

Comment 1

[Josh Aas]

Attachment: fix v1.0

Comment 2

[Josh Aas]

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

James, can you make sure this doesn't break your click-to-play test on mobile?

Comment 3

[Josh Aas]

try run for fix v1.0

https://tbpl.mozilla.org/?tree=Try&rev=f3da1b54aee8

Comment 4

[Josh Aas]

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

I ran James's Flash click-to-play test on Android with this patch and it works fine. Switching review to bsmedberg.

Comment 5

[Josh Aas]

It's hard to write a test for just this bug because we also have a bug in which we don't create an object frame for stream-based instantiations. I'm writing a test for both.

Comment 6

[Benjamin Smedberg]

Why were they aborted? I'm a bit confused about what the sequence of events is here.

Comment 7

[Josh Aas]

Right now we mark the stream as delivered (we call it something else, mSrcStreamLoadInitiated) when ::OnStartRequest is called on nsObjectLoadingContent. If the plugin fails to load because it doesn't have an object frame we'll never load the stream again once it does get an object frame. So an object frame gets created, the instance is created successfully, but it never gets the src stream data.

Comment 8

[Josh Aas]

Note that because of the other bug I mentioned, in which we don't create an object frame when we should, it is hard to see how this fix alone resolves anything. We don't get to a point where we'd need to re-load the src stream without the object frame fix. I want to deal with the issues separately though, and this one first.

Comment 9

[Benjamin Smedberg]

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

I think you need to update the comment in nsObjectLoadingContent.h, perhaps with lots more details. I don't yet understand how we actually deal with the case where the plugin "fails to load"... do we cache the data and feed it to the plugin later?

Comment 10

[Josh Aas]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)

> I think you need to update the comment in nsObjectLoadingContent.h, perhaps
> with lots more details. I don't yet understand how we actually deal with the
> case where the plugin "fails to load"... do we cache the data and feed it to
> the plugin later?

Any caching would happen at the network layer, in response to an aborted binding. I don't know if it actually caches or not in that situation, but it isn't relevant on this level.

Instantiation normally attempts to load src data (in nsPluginHost::InstantiateEmbeddedPlugin). We don't want to do that if we're loading from stream input at the DOM level, which is what this variable is tracking. If we did we'd feed duplicate streams to the plugin. If we fail to load here, then the next instantiation will try again in nsPluginHost::InstantiateEmbeddedPlugin, when nsObjectLoadingContent::SrcStreamLoading is false.

Comment 11

[Benjamin Smedberg]

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

r=me with the comment you pasted in IRC

Comment 12

[Josh Aas]

Attachment: fix v1.1

Updated comment.

Comment 13

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/34c94825a183