515549 - Page Info > Media tab doesn't preview <embed> tags for plugins like flash

Status: RESOLVED INVALID

(Firefox :: Page Info Window, defect)

Description

[-fullmetaljacket-]

as the summary stated.

Reproducible: Always

Steps to Reproduce:
1. Go to the page (URL on this bug or any page with flash)
2. Choose Tools > Page Info > Media
3. Select the flash from the list of addresses.
Actual Results:  
"Media Preview" shows nothing or a broken image.  "Dimensions" shows 0px x 0px.


Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20090909 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20090909041701

Comment 1

[Philip Chee]

I thought that this is a DUP and several are close but I couldn't find an exact match.

Comment 2

[-fullmetaljacket-]

yup i searched bugzilla before filing this one and the closest (same problem) i see is the SVG preview (bug 453703).

Comment 3

[Tanner M. Young [:tmyoung]]

I couldn't find another copy of this bug, and this is a valid request--particularly as we now support ogg video.

Comment 4

[Tanner M. Young [:tmyoung]]

Attachment: WIP No. 1

This is a work in progress.  I openly admit this isn't good code, but it's something of a start.  I have experimented both with previewing flash and css generated content at the same time (Bug 549150).  This is what I came up with.  I will come back to this patch sometime in the future, but if someone wants to take this over before I can, please do.

Comment 5

[Philip Chee]

[Not a review]

+    Array.forEach(computedStyle.getPropertyCSSValue("content"), function (url) {
+      if (url.primitiveType == CSSPrimitiveValue.CSS_URI)
+        addImage(url.getStringValue(), gStrings.mediaBGImg, gStrings.notSet, elem, true);
+    });

We already detect HTMLEmbedElement. What does this do here? Is it possible to have multiple content?

+  else if (item instanceof HTMLEmbedElement && isProtocolAllowed) {
+    newImage = document.createElementNS("http://www.w3.org/1999/xhtml", "object");
+    var embed = document.createElementNS("http://www.w3.org/1999/xhtml", "embed");

No need to wrap the embed in an object (The Satay method), just use embed directly or convert it to object.

+    newImage.appendChild(document.createElementNS("http://www.w3.org/1999/xhtml", "param"));
You don't use this.

+    embed.allowfullscreen = item.allowfullscreen;
This doesn't make sense in a preview.

+    embed.allowscriptaccess = item.allowscriptaccess;
There won't be any scripts accessing this embed inside the PageInfo window.

Comment 6

[Tanner M. Young [:tmyoung]]

(In reply to comment #5)
> [Not a review]

But much appreciated!

> We already detect HTMLEmbedElement. What does this do here? Is it possible to
> have multiple content?

That's a different item not relevant to this bug...

> No need to wrap the embed in an object (The Satay method), just use embed
> directly or convert it to object.

I did that because Youtube videos wouldn't play in the preview (or anything else for that matter).  What I have since found is that item.flashvars is undefined, even though the DOM inspector can find it.  Since Youtube defaults to the Satay method for loading videos in Firefox, then nothing gets passed to Page Info, if you nest the embed in an object, it works perfectly, or if you pass item itself to newImage it works perfectly, except that the video is pulled out of the window into Page Info, which is not a solution.

No pages on the internet explain how to pass flashvars via Javascript using the Satay method.  I like the Satay method, but if it doesn't work, then the point is moot.

If we knew how the DOM inspector found the attribute, then we could probably pass it, but accessing by item.flashvars doesn't work, and I can't think of another way of communicating with it.

> +   
> newImage.appendChild(document.createElementNS("http://www.w3.org/1999/xhtml",
> "param"));
> You don't use this.

No, and I don't know why I did...

> +    embed.allowfullscreen = item.allowfullscreen;
> This doesn't make sense in a preview.

No it doesn't...

> +    embed.allowscriptaccess = item.allowscriptaccess;
> There won't be any scripts accessing this embed inside the PageInfo window.

Would any extensions need or want to access this?


With the flashvars issue resolved, I would have working patch.

Comment 7

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 1.0

If I remember that getAttribute and setAttribute do what I need, then suddenly everything works...

I have made all of the changes to the WIP that Philip suggested.  I haven't done anything with the HTMLObjectElement since all of the flash examples I found were embeds nested in objects or standalone embeds.

This is also fixes the case of SVGs included as embed tags for Bug 453703.

Comment 8

[Philip Chee]

>> +    embed.allowscriptaccess = item.allowscriptaccess;
>> There won't be any scripts accessing this embed inside the PageInfo window.
> Would any extensions need or want to access this?

I should hope not. Besides they can access these from the content page directly.

However this brings to mind another question. Is this flash running with chrome privileges? I think a security review might be appropriate here. In any case (speaking as the current Flashblock maintainer) I would prefer allowscriptaccess not to be turned on.

By the way, the Adobe/Macromedia knowledgebase says that it is "FlashVars" IniCaps:
<http://kb2.adobe.com/cps/164/tn_16417.html>
But a quick google shows that lowercase is widespread in the wild as well.

Comment 9

[Tanner M. Young [:tmyoung]]

(In reply to comment #8)
> I should hope not. Besides they can access these from the content page
> directly.

Glad I removed it...

> However this brings to mind another question. Is this flash running with chrome
> privileges? I think a security review might be appropriate here. In any case
> (speaking as the current Flashblock maintainer) I would prefer
> allowscriptaccess not to be turned on.

I don't think it is, but let's certainly make sure.

> By the way, the Adobe/Macromedia knowledgebase says that it is "FlashVars"
> IniCaps:
> <http://kb2.adobe.com/cps/164/tn_16417.html>
> But a quick google shows that lowercase is widespread in the wild as well.

I've seen it both ways, all of the cases I have seen have it lowercase--perhaps for XHTML compliance.  I personally prefer the lowercase as it is consistent with XHTML, but if it violates or breaks a convention, I will gladly change it.

Comment 10

[Philip Chee]

No I mean this line:
>+    newImage.setAttribute("flashvars",item.getAttribute("flashvars"));
...........................................................^^^^^^^^^
Have you applied this to a test case where the content webpage uses IniCaps "FlashVars"?

Comment 11

[Tanner M. Young [:tmyoung]]

(In reply to comment #10)
> No I mean this line:
> >+    newImage.setAttribute("flashvars",item.getAttribute("flashvars"));
> ...........................................................^^^^^^^^^
> Have you applied this to a test case where the content webpage uses IniCaps
> "FlashVars"?

From testing http://www.permadi.com/tutorial/flashVars/index.html, it works.

Comment 12

[Tanner M. Young [:tmyoung]]

(In reply to comment #8)
> However this brings to mind another question. Is this flash running with chrome
> privileges? I think a security review might be appropriate here.

One is in the works right now.

Comment 13

[Tanner M. Young [:tmyoung]]

Comment on attachment 449972 [details] [diff] [review]
Patch v. 1.0

Dao--understandably--doesn't have time to look at this at the moment, and since we are waiting on the security review, I am removing the review flag for the moment.

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 449972 [details] [diff] [review]
Patch v. 1.0

>+    newImage.setAttribute("flashvars",item.getAttribute("flashvars"));

Is this for generic <embed> or specifically flash? If flash only should you add more checks to make sure that's what you've got? But if it is flash only why the preferential treatment?

I'm dubious about the usability/user experience here, but I'll leave that to the FF module owners/peers. From a security PoV, though, I'm really unhappy running plugins in chrome. Primary worries:
 - the plugin might script the chrome-privileged container
 - the plugin might be confused about its origin and assume local file

The first is rather obvious and Philip mentioned it already. Adobe promises flash won't script out unless you give it the magic parameter, but other plugins have no such feature and even with Flash I'm not sure how comfortable I am relying on their checks for our security.

The second we've had a smattering of problems with over the years because some plugin vendors assume if it's not a web protocol they recognize or there's no host then it must be a local file, and then they allow the plugin to read/write any file the user can.

I assume the reason you're doing this is so people can distinguish between various movies so they download the right one, not because people actually want to watch them or play games in that awkward little space. If that's the case you might be able to accomplish the goal by grabbing a snapshot of the <embed> and drawing it into a <canvas> in the pageInfo dialog. That'd be a very safe way to go, and would work for <object> too.

  [Incidentally, there's an unrelated minor UI problem with the
   "block" checkbox: it still says (and does) block images from
   the selected site even on a non-image.]

If you're completely unsatisfied with anything less than a 100% live plugin, and the UI guys approve that, then you're going to have to
 - put it in a <browser> or <iframe>
 - with the attribute type="content"
 - and programmatically turn off scripts in that frame's docshell.

Here's an example of microsummaries doing a similar thing (obviously you wouldn't want to disable plugins for this bug, and you wouldn't want it hidden):
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsMicrosummaryService.js#1997

Comment 15

[Daniel Veditz [:dveditz]]

> But if it is flash only why the preferential treatment?

Meant "if it is _NOT_ flash only..."

Comment 16

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 2.0

This creates the iframe, protects it, and then inserts the <embed> element.

The main issue with the patch is that the iframe can cover the scrollbars or otherwise bleed into the Page Info window, and I can't figure out which css option will mitigate that; however, it should fit the security specifications requested earlier.

Comment 17

[Philip Chee]

A few micro-nits. Feel free to ignore me.

+  var isEmbed = false;
Perhaps something more generic like shouldSanitize?

+    newImage = document.createElement('iframe');
Use "double quotes" consistently.

+    newImage.style.width = (parseInt(width) + 20) + "px";
+    newImage.style.height = (parseInt(height) + 20) + "px";
20px is a magic number, could you document what this does? I presume it is something to do with the scrollbars. Does this value work for all supported operating systems? Popular third party themes?

+    setTimeout(loadFrameContents,200,embed);
One space after each comma. 200ms seems rather arbitrary, how did you derive this number?

Comment 18

[Tanner M. Young [:tmyoung]]

(In reply to comment #17)
> +  var isEmbed = false;
> Perhaps something more generic like shouldSanitize?

Yes, and I've changed it already.

> +    newImage = document.createElement('iframe');
> Use "double quotes" consistently.

Yes...Sorry...Fixed.

> +    newImage.style.width = (parseInt(width) + 20) + "px";
> +    newImage.style.height = (parseInt(height) + 20) + "px";
> 20px is a magic number, could you document what this does? I presume it is
> something to do with the scrollbars. Does this value work for all supported
> operating systems? Popular third party themes?

Only tested on windows.  If there is a more elegant solution to prevent the scrollbars (simply doing something like overflow:visible didn't have any effect), I am more than happy to replace it; this was the least complicated solution that I found without setting over a dozen css values.

> +    setTimeout(loadFrameContents,200,embed);
> One space after each comma. 200ms seems rather arbitrary, how did you derive
> this number?

It is arbitrary; if there is a better value, I am glad to use it (in the new patch it's 20ms since that's a lot faster and still works).  The important thing is that we need a timeout value; we can't attach to the iframe's load event since we are loading a blank tab and it doesn't have event handlers.  If you don't set the iframe source to "" or try to use the iframe's event handlers, the iframe doesn't paint or the embed is not pushed to the frame.

This also brings up another question (that may not be worth filing a bug).  Why is it 16 ms on the doGrab timeout?  I looked at the cvs blame for that, and it appears that it came from Bug 195492 (perhaps Bug 181764?).  I tested it with 2ms timeouts and worked with my machine.  I ask only because the primary Page Info feedback is that the media tab for a page with 100 images takes 1.6 seconds to load, which when you run View Image Info on the last image, it must wait for the whole page to load.  Can we change this to a smaller value or implement it differently?  Setting it to 2ms worked for me, and in the original Bug 181764 patch, it was set to 1ms.

Comment 19

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 2.1

Patch with corrections from the above comments and other irc discussion.

Comment 20

[Florian Quèze [:florian]]

(In reply to comment #18)

> This also brings up another question (that may not be worth filing a bug).  Why
> is it 16 ms on the doGrab timeout?

There's a minimal value for the timeout, currently 10ms (added in bug 123273). See also https://developer.mozilla.org/en/window.setTimeout#Minimum_delay_and_timeout_nesting

Currently, the minimal value is 10, so I guess changing the 16ms constant to 10 would save a few milliseconds.
I think you can speed up the process a bit by increasing the number of DOM nodes processed at a time by the doGrab function (change the "50" constant).
Or you can find a 0ms solution similar to what is used in some tests.

> when you run View Image Info on the last image, it must wait for the whole
> page to load.  Can we change this to a smaller value or
> implement it differently?

What about adding the specific image that should be selected in the image list before starting to process the DOM tree recursively? I think it would eliminate the annoying wait in the "View Image Info" case.

Comment 21

[Philip Chee]

> It is arbitrary; if there is a better value, I am glad to use it (in the new
> patch it's 20ms since that's a lot faster and still works).  The important
> thing is that we need a timeout value; we can't attach to the iframe's load
> event since we are loading a blank tab and it doesn't have event handlers.  If
> you don't set the iframe source to "" or try to use the iframe's event
> handlers, the iframe doesn't paint or the embed is not pushed to the frame.

http://mxr.mozilla.org/mozilla-central/search?string=loadOrDisplayResult

The toolkit error console uses a load event listener on their iframe:

  gEvaluator.addEventListener("load", loadOrDisplayResult, true);
....
function evaluateTypein()
{
  gCodeToEvaluate = gTextBoxEval.value;
  // reset the iframe first; the code will be evaluated in loadOrDisplayResult
  // below, once about:blank has completed loading (see bug 385092)
  gEvaluator.contentWindow.location = "about:blank";
}

function loadOrDisplayResult()
{
  if (gCodeToEvaluate) {
    gEvaluator.contentWindow.location = "javascript: " +
                                        gCodeToEvaluate.replace(/%/g, "%25");
    gCodeToEvaluate = "";
    return;
  }
....

Comment 22

[Philip Chee]

This also brings up another question (that may not be worth filing a bug).  Why
is it 16 ms on the doGrab timeout?  I looked at the cvs blame for that, and it

I guess it's an arbitrary value that is or was a reasonable compromise at that time. I don't know if it is worth replacing the iterator with a generator and a setTimeout() of 0.

https://developer.mozilla.org/en/new_in_javascript_1.7#Generator_Example

Also see dbaron's blog post on using postMessage:

http://dbaron.org/log/20100309-faster-timeouts

Comment 23

[Daniel Brooks [:db48x]]

(In reply to comment #22)
> This also brings up another question (that may not be worth filing a bug).  Why
> is it 16 ms on the doGrab timeout?  I looked at the cvs blame for that, and it
> 
> I guess it's an arbitrary value that is or was a reasonable compromise at that
> time. I don't know if it is worth replacing the iterator with a generator and a
> setTimeout() of 0.

It's an arbitrary value that I choose ages ago, as is the number of elements processed at a time. Computers are a bit faster than they used to be, so I don't see any reason why we can't make the timeout smaller and also the chunk size larger.

Comment 24

[Tanner M. Young [:tmyoung]]

For the general performance question, I have filed Bug 573603.

Comment 25

[Philip Chee]

> Computers are a bit faster than they used to be.
Not to mention that we now JIT chrome.

Comment 26

[Tanner M. Young [:tmyoung]]

(In reply to comment #16)
> The main issue with the patch is that the iframe can cover the scrollbars or
> otherwise bleed into the Page Info window, and I can't figure out which css
> option will mitigate that; however, it should fit the security specifications
> requested earlier.

This is caused by Bug 130078, so when that is fixed the overlapping issue will go away on its own.

Comment 27

[Jennifer Morrow [:Boriss] (UX)]

Tanner M. Young mentioned to me that there may be an issue of sound from a media tab element playing on top of other sound in the browser.  I recommend ignoring this and letting the sound overlay.  The media tab is not a media player - finding that an element creates sound is likely all that is needed.  The alternative is a workaround where either the media tab or browser is muted, which degrades the experience of both.

Comment 28

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 2.2

Fixed a small error and updated the patch to work after Bug 377349 landed.

Comment 29

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 2.3

This precludes potential problems in the future by having the element created as part of the iframe rather than part of chrome document itself, but it requires moving the createElementNS and attribute definitions after the iframe has been appended to Page Info.

Comment 30

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 2.4

Fixed syntax error.

Comment 31

[Daniel Brooks [:db48x]]

Comment on attachment 453934 [details] [diff] [review]
Patch v. 2.4

Not quite there yet. The dimensions are showing up as NaN×NaN, which is pretty silly. Perhaps the width/height properties are undefined when the proper plugin is absent and the null plugin takes over?

Package the shouldSanitize stuff into a function, and simply call it where needed.

loadFrameContents should simply be an inline function.

No space is needed between a function name and the open parenthesis either.

Comment 32

[Tanner M. Young [:tmyoung]]

Attachment: Patch v. 2.5

(In reply to comment #31)
> (From update of attachment 453934 [details] [diff] [review])
> Not quite there yet. The dimensions are showing up as NaN×NaN, which is pretty
> silly.

Is it because you are missing a plugin?  Can you provide a url? (Since all of the ones I've tested work so far.)

> Package the shouldSanitize stuff into a function, and simply call it where
> needed.

Moved it to a function.

> loadFrameContents should simply be an inline function.

Done.

> No space is needed between a function name and the open parenthesis either.

Done.

Comment 33

[Daniel Brooks [:db48x]]

(In reply to comment #32)
> (In reply to comment #31)
> > (From update of attachment 453934 [details] [diff] [review] [details])
> > Not quite there yet. The dimensions are showing up as NaN×NaN, which is pretty
> > silly.
> 
> Is it because you are missing a plugin?  Can you provide a url? (Since all of
> the ones I've tested work so far.)

Very probably. I had loaded something from video.google.com, but don't actually have flash installed.

Comment 34

[Brandon Sterne (:bsterne)]

Attachment: location-setting SWF

Here's a swf that tries to set the location using either navigateToURL or ExternalInterface.call(location.replace).

Here's the Flex I used to generate the swf if someone wants to modify it... I can't get either to set the location to a javascript: URL.

<mx:Application xmlns:mx="http://www.adobe.com/2006/mxml" layout="absolute">
    <mx:Script>
        <![CDATA[
            import flash.net.navigateToURL;
            import mx.controls.Alert;
            private function navToURL() : void {
            	try {
	                navigateToURL( new URLRequest( newUrl.text ), "_self" );
	            }
	            catch (e:Error) { Alert.show(e.toString()); }
            }
            private function extNavURL() : void {
            	if (ExternalInterface.available) {
          			try {
			            ExternalInterface.call("window.open", newUrl.text);
          			}
					catch (e:Error) { Alert.show(e.toString()); }
				}
            }
        ]]>
    </mx:Script>
    <mx:VBox top="20" left="20">
		<mx:HBox>
		    <mx:TextInput
		        id="newUrl"
		        text="javascript:alert(1);" />
		</mx:HBox>
		<mx:HBox>
		    <mx:Button
		        label="navigateToURL"
		        click="navToURL()" />
		        
		    <mx:Button
		        label="location.replace"
		        click="extNavURL()" />
		</mx:HBox>		
    </mx:VBox>
</mx:Application>

Comment 35

[Brandon Sterne (:bsterne)]

(In reply to comment #34)
>                       try {
>                         ExternalInterface.call("window.open", newUrl.text);
>                       }

And the attached SWF does have the correct call to location.replace (just forgot to update my clipboard).

Comment 36

[Tanner M. Young [:tmyoung]]

Because of several problems with iframes--that may be fixed in other bugs--development for this is stalled.  There is some discussion about other changes to Page Info that would make this much simpler, but this would need to wait on such changes, and all dependent bugs would likewise need to wait on such changes.

I no longer have time to actively work on this regardless of whether the iframe issues are resolved.  Anyone is free to take the bug and treat the above patch as a WIP with the understanding that the patch works but does not guarantee plug-in security.

Comment 37

[Tanner M. Young [:tmyoung]]

After talking to dveditz, we should be fine--in theory--if we place the content in a <browser type=content></browser> tag in XUL, which should resolve the security concerns that were raised with this patch initially.

The issues raised in Comment #36 are now obsolete or fixed, but I still do not have the time or development environment to see this patch to completion.  I am available for feedback and other input, but not to finish writing or testing the patch.

Also, by now, parts of the patch may be obsoleted by Page Info changes in the intervening time, but that should be minimal.

Comment 38

[Mats Palmgren (inactive)]

We're in the process of removing support for plugins (bug 1677160), so I think this bug is irrelevant now.

Comment 39

[skierpage]

(In reply to Mats Palmgren (:mats) from comment #38)

We're in the process of removing support for plugins (bug 1677160), so I think this bug is irrelevant now.

Should bug 574545 (and its dependent bug 453703) be un-depended from this now that it's INVALID?

Comment 40

[Florian Quèze [:florian]]

(In reply to skierpage from comment #39)

(In reply to Mats Palmgren (:mats) from comment #38)

We're in the process of removing support for plugins (bug 1677160), so I think this bug is irrelevant now.

Should bug 574545 (and its dependent bug 453703) be un-depended from this now that it's INVALID?

Bug 453703 still seems valid, and I removed the dependency. I don't think bug 574545 is relevant anymore, and I closed it.