Closed Bug 484992 Opened 16 years ago Closed 14 years ago

Multiple GET requests to flash content if using OBJECT tag with data attribute

Categories

(Core Graveyard :: Plug-ins, defect, P1)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
Windows XP
Type:
defect

Tracking

(blocking2.0 beta8+, blocking1.9.2 -, status1.9.2 .13-fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b7
Tracking Flags:
Tracking Status
blocking2.0 --- beta8+
blocking1.9.2 --- -
status1.9.2 --- .13-fixed

People

(Reporter: ejmalone, Assigned: bzbarsky)

References

Details

Attachments

(1 file)

and bug 604420. Don't start a new network request when instantiating the plug-in for <object>s.
8.31 KB, patch
jaas
: review+
dveditz
: approval1.9.2.13+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7

We noticed that 2 GET requests for flash from one piece of markup were being requested. It is occurring because the markup generated by SWFObject is of the form (extraneous attributes removed)
<object data="xx" type="application/x-shockwave-flash" width="yy" height="zz"></object>

I've confirmed that markup of the following form does not cause the bug.
<embed src="xx" type="application/x-shockwave-flash" width="yy" height="zz" />

I've tested Firefox 2, and it does not load the flash content twice.

Reproducible: Always

Steps to Reproduce:
1. Create an html file (test_object_code.html in my case) with these contents
<html>
<body>
<object data="http://vimeo.com/moogaloop_local.swf" type="application/x-shockwave-flash" width="304" height="30"></object>
</body>
</html>
2. Using a network analyzer, monitor the requests
3. Compare with
<html>
<body>
<embed src="http://vimeo.com/moogaloop_local.swf" type="application/x-shockwave-flash" width="304" height="30"  />
</body>
</html>
Actual Results:  
GET http://ubuntu:3000/test_object_code.html
200 OK

GET http://vimeo.com/moogaloop_local.swf
200 OK

GET http://vimeo.com/moogaloop_local.swf
200 OK



Expected Results:  
GET http://ubuntu:3000/test_object_code.html
200 OK

GET http://vimeo.com/moogaloop_local.swf
200 OK


I used Vimeo as an example, I'm not affiliated with them. I used Fiddler to monitor the traffic with the proxy settings in Firefox set to 127.0.0.1:8888. ubuntu:3000 is a local host running Rails and hosting the test file.
Component: General → Plug-ins
Product: Firefox → Core
QA Contact: general → plugins
We're seeing this issue on YouTube as well now.  We are moving away from using SWFobject and we're trying to refactor our code to be more of the form:

  <object type="application/x-shockwave-flash" data="http://s.ytimg.com/yt/swf/watch_as3-vfl148857.swf" width="100%" height="100%" >
    <param name="flashvars" value="">
  </object>

This, however, as the previous comment indicates triggers two HTTP requests.

Any thoughts?
This should probably be filed under Core->Networking?  I can't change it myself - can someone do so that the right people look at this bug?
I'm guessing you mean two HTTP requests for the .swf file? After the first two requests do we load it from cache, or do we keep re-requesting it?
Hmm.  So the first load is from nsObjectLoadingContent.  The second load is from:

#4  0x1275a9be in NS_NewChannel (result=0xbfffc5dc, uri=0x2026bec0, ioService=0x9276a0, loadGroup=0x1f198940, callbacks=0x0, loadFlags=0) at nsNetUtil.h:181
#5  0x1274d3c5 in nsPluginHost::NewEmbeddedPluginStream (this=0x2020d570, aURL=0x2026bec0, aOwner=0x2020cbc0, aInstance=0x1f29b760) at /Users/bzbarsky/mozilla/vanilla/mozilla/modules/plugin/base/src/nsPluginHost.cpp:4662
#6  0x1274e05e in nsPluginHost::InstantiateEmbeddedPlugin (this=0x2020d570, aMimeType=0x2027aa58 "application/x-shockwave-flash", aURL=0x2026bec0, aOwner=0x2020cbc0) at /Users/bzbarsky/mozilla/vanilla/mozilla/modules/plugin/base/src/nsPluginHost.cpp:2498
#7  0x127504bf in nsPluginStreamListenerPeer::OnStartRequest (this=0x1f29b700, request=0x2026c070, aContext=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/modules/plugin/base/src/nsPluginHost.cpp:1229
#8  0x12d9d9a3 in nsObjectLoadingContent::OnStartRequest (this=0x2026bc2c, aRequest=0x2026c070, aContext=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/base/src/nsObjectLoadingContent.cpp:695

That looks wrong.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Well... with <embed> we trust the DOM type="application/x-shockwave-flash" annotation. With <object> I think we check the MIME type before continuing. I'm guessing that we check the MIME type, then hand the *URL* off to the plugin host, instead of handing off the channel/data.
Hrm.  So nsPluginHost::InstantiatePluginForChannel sets up a nsPluginStreamListenerPeer with a null mInstance.  Then in nsPluginStreamListenerPeer::OnStartRequest we end up doing:

1212   // if we don't have an nsIPluginInstance (mInstance), it means
1213   // we weren't able to load a plugin previously because we
1214   // didn't have the mimetype.  Now that we do (aContentType),
1215   // we'll try again with SetUpPluginInstance()
1216   // which is called by InstantiateEmbeddedPlugin()

Is there a reason that InstantiatePluginForChannel() doesn't actually instantiate anything?  Like, say, calling SetupPluginInstance?  And maybe FindStoppedPluginForURL, I guess, if that's needed?  Or is there other stuff from InstantiateEmbeddedPlugin that's needed here?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.2: --- → ?
Benjamin, we hand the data off to the plugin host.  But it decides to ignore that and start a new load anyway....
We'd take a reviewed and baked patch, but I don't think that this blocks.
blocking1.9.2: ? → -
status1.9.2: ? → wanted
So just to be clear....  I'm not familiar with this code, so much.  Josh says he's not either.  Benjamin, do you know anything about it?  jst, what about you?

This bug is only going to get fixed if someone steps up.  I can be that someone if there's no one better qualified, I guess...

I do think this should be a very high priority to fix.
I don't know this code particularly well either, but I can hack on it if need be. Do we know if this is a regression? If not, I don't think I'd block on this either...
Well, it's a regression from the "make acid2 work" <object> changes.  But the upshot is that any time someone uses <object> for a plug-in we do two loads...  I do think we should block 1.9.3 on that.
1.9.3, yes, I for some reason was thinking 1.9.2.
blocking2.0: ? → beta1
Well, beltzner's comment was about 1.9.2.  ;)
How does <embed> work? Does the object loading content not do the initial load, or do we pass it off to the plugin correctly?
FWIW, I've gotten a bit more familiar with this code recently, and I could probably fix this bug in, say, 8 weeks, but I don't think it's important enough to delay OOPP work.
For the <embed> case, as long as we're given an @type we use that type to determine handling.  If it's a plug-in handled type we just have the plugin host do the load directly.

jst, I guess my question is whether you want to take this or whether I should.  ;)
blocking2.0: beta1+ → beta2+
At this point I don't think we should block 2.0 on this, but I'd still take a patch.
blocking2.0: beta2+ → -
I think that's the wrong call.  This is a serious problem that I strongly feel we need to fix, and it won't get fixed by magic.  Someone needs to be actively tasked with working on it.

I guess I'll drop some other performance work and look into this...
Assignee: nobody → bzbarsky
Priority: -- → P1
Blocks: 604420
Alright, this is the underlying reason for bug 604420.

And it looks like the original checkin for InstantiatePluginForChannel has:

  // XXX do we need to look for stopped plugins, like InstantiateEmbeddedPlugin
  // does?

I will now claim that it's just always been buggy.... ;)
Comment on attachment 484601 [details] [diff] [review]
and bug 604420.  Don't start a new network request when instantiating the plug-in for <object>s.

This is the safest patch I could come up with.  What it does is just make sure that we don't actually open new streams when instantiating from an existing stream's OnStartRequest.

The other option is to just back out the InstantiateEmbeddedPlugin call there and always just call SetUpPluginInstance; the call in question was added for bug 54437 and I _think_ might not be relevant anymore, as long as I leave the "don't try instantiating, just open a stream, if we have a URI but not type" gunk that patch added in InstantiateEmbeddedPlugin.  Or maybe even if I remove it, since we have no more default plugin.... not sure.  But that seemed like a riskier fix.  I'm happy to file a followup bug on doing that, though.

Another thing that I considered which seemed risky-ish is moving the plugin instance CreateWidget etc stuff in OnStartRequest into the |else| case there, since DoInstantiateEmbeddedPlugin already handles that.  But I think it's harmless to do it twice (correct me if I'm wrong!), and we're already doing it twice for <object> on trunk....  This becomes irrelevant if we remove the DoInstantiateEmbeddedPlugin call per above.

josh, I _think_ I understand how this stuff is working now, but please double-check things carefully?  This code scares me.  :(

Also, if you have an idea of how to write a test for this, I'd love to know.
Attachment #484601 - Flags: review?(joshmoz)
Blocking betaN, per bug 604420.
blocking2.0: - → betaN+
Whiteboard: [need review]
Boris... is this going to fix the problems mentioned in bug 604420, especially the problem with AdBlock Plus?
Gary, Boris already said that in bug 604420 - yes, this issue is the root cause for bug 604420 and fixing it will fix bug 604420 as well.
Boris: Is there a tryserver build with this patch in it?  I need to check whether this also resolves the bug for the Stop Autoplay v1.0 extension, which is also affected by bug 604420.
There isn't.  Which OS do you want a build for?  I can spin one up right now.
Er, wait.   I lied.  I did push this to try, since it's scary and all.  ;)

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/bzbarsky@mozilla.com-67046f5e402b/ has the builds.
Boris... Gave your tryserver version a try and it appears to work fine alongside AdBlock Plus.  I was able to view all the videos reported in bug 604420 plus a few I found along the way.  

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101019 Firefox/4.0b8pre - Build ID: 20101019235330
It solves AdBlock Plus problems for me too
Tried many sites with Flash and all are working just fine with ABP.  Unless there are other add-ons that need to be tested with this patch I say let's land this baby.
(In reply to comment #30)
> I say let's land this baby.

Nice to know it works, but the patch isn't actually reviewed yet.
(In reply to comment #31)
> (In reply to comment #30)
> > I say let's land this baby.
> 
> Nice to know it works, but the patch isn't actually reviewed yet.

Then let's put the pedal to the metal on this one.
How about not spamming my inbox until Josh reviews?  ;)
blocking2.0: betaN+ → beta8+
Whiteboard: [need review] → [needs review josh]
Thanks, Boris.  Unfortunately, it doesn't solve the problem for Stop Autoplay, so I may have to open a new bug report specifically for that.
Yes, please.  New bug, with steps to reproduce.
To comment 34. Isn't it better to land this and get it fixed as it affects many users that use ABP and would like to view sites with flash content. The auto play problem is minor compared to this.
Gary, see comment 33 and comment 35...
(In reply to comment #35)
> Yes, please.  New bug, with steps to reproduce.

OK.  Filed bug 606077
Comment on attachment 484601 [details] [diff] [review]
and bug 604420.  Don't start a new network request when instantiating the plug-in for <object>s.

All of this plugin instantiation code is in need of some serious cleanup, hopefully I'll have time to do that some time. Thanks for the fix Boris!
Attachment #484601 - Flags: review?(joshmoz) → review+
Whiteboard: [needs review josh] → [need landing]
http://hg.mozilla.org/mozilla-central/rev/54b9b52337b7
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b8
Comment on attachment 484601 [details] [diff] [review]
and bug 604420.  Don't start a new network request when instantiating the plug-in for <object>s.

I think it's worth considering this for 1.9.2.   This is as safe as any patch to this code gets, and fixes without fixing this it's easy to write content policies that will cause plug-ins to fail to load, even on 1.9.2.
Attachment #484601 - Flags: approval1.9.2.12?
I filed bug 606641 on cleaning this stuff up post-2.0.
(In reply to comment #41)
> I think it's worth considering this for 1.9.2.   This is as safe as any patch
> to this code gets, and fixes without fixing this it's easy to write content
> policies that will cause plug-ins to fail to load, even on 1.9.2.

I hope it fixes this issue on 1.9.2: https://adblockplus.org/forum/viewtopic.php?f=1&t=5992 (given that only one website is affected and it is a pretty complicated one I didn't get around to filing a bug yet).
Comment on attachment 484601 [details] [diff] [review]
and bug 604420.  Don't start a new network request when instantiating the plug-in for <object>s.

Approved for 1.9.2.12, a=dveditz for release-drivers
Attachment #484601 - Flags: approval1.9.2.12? → approval1.9.2.12+
Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/12d0d5312b11

Wladimir, let me know whether that fixes the issue you mention?
status1.9.2: wanted → .12-fixed
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: