Closed Bug 729673 Opened 8 years ago Closed 8 years ago

allow for plugin instance src stream load retry

Categories

(Core :: Plug-ins, defect)

12 Branch
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch fix v1.0 (obsolete) — Splinter Review
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?
Attachment #600682 - Flags: review?(snorp)
try run for fix v1.0

https://tbpl.mozilla.org/?tree=Try&rev=f3da1b54aee8
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.
Attachment #600682 - Flags: review?(snorp) → review?(benjamin)
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.
Why were they aborted? I'm a bit confused about what the sequence of events is here.
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.
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.
Summary: don't mark plugin src stream as delivered unless it was actually delivered → allow for plugin instance src stream load retry
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?
(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 on attachment 600682 [details] [diff] [review]
fix v1.0

r=me with the comment you pasted in IRC
Attachment #600682 - Flags: review?(benjamin) → review+
Attached patch fix v1.1Splinter Review
Updated comment.
Attachment #600682 - Attachment is obsolete: true
Attachment #601011 - Flags: review?(benjamin)
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/34c94825a183
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 728125, 727427
Attachment #601011 - Flags: review?(benjamin)
You need to log in before you can comment on or make changes to this bug.