Closed Bug 395110 Opened 17 years ago Closed 17 years ago

server provided content-type takes precedent over type-attribute

Categories

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

Product:
Core Graveyard
Component:
Plug-ins
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.9

People

(Reporter: samuel.sidler+old, Assigned: jst)

References

(
URL
)

Details

(Keywords: regression, testcase)

Attachments

(3 files, 2 obsolete files)

reduced testcase
842 bytes, text/html
Details
Feedback submitted to Apple website
2.86 KB, text/plain
Details
Updated fix.
1.34 KB, patch
sicking
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
I can't seem to reproduce this when I save the page onto my local disk, but it definitely happens on Apple's site. STR: 1. Go to http://events.apple.com.edgesuite.net/s83522y/event/index.html?internal=g4h5jl83a 2. Click on "Watch the Keynote Address" Actual Results: Nothing happens. Expected Results: Load Quicktime with the correct URL. Tested using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007090404 Minefield/3.0a8pre This works fine on branch.
Flags: blocking1.9?
My CSS-fu is weak, but it's in a nested DIV. Since the CSS isn't local, it makes sense that the page might work when its CSS rules (remote, at least) no longer apply. I see this too on Windows XP SP 2 with today's trunk Minefield. No errors/exceptions are thrown in the Error Console. Dan, do you have cycles/interest in helping us triage this? Pretty sure it'll be a layout bug.
OS: Mac OS X → All
Hardware: PC → All
Attached file reduced testcase —
Expected behavior of testcase: - Should load a little movie applet, which is linked to wiki.mozilla.org. (Branch behavior) Observed behavior, on Trunk: - Loads an image instead of movie applet. Image is *not* linked / clickable. I don't think this is a layout bug. I think it's due to how Trunk is handling "video/quicktime"... I'm guessing this because if I remove this line m.setAttribute("type","video/quicktime"); from the testcase, then Branch behaves like Trunk. (It just shows a non-linked image.)
If you want to save the testcase locally, *don't* save it as "Web Page, Complete", or else you'll get some extra junk. Instead, save it as "Web Page, HTML Only." Or right-click the testcase link (here: attachment 279857 [details]) and choose "Save link as..."
Keywords: testcase
This was easier to track down using Camino (downloading builds was easier), so that's where I did it. This broke between Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/2005092108 Camino/1.0+ and Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/2005092208 Camino/1.0+ Yes, that's 2005. Regression range: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-09-21+08&maxdate=2005-09-22+08&cvsroot=%2Fcvsroot Possibly a regression from bug 1156?
I'm going to move this to Core::Plug-ins because I bet no one's triaging Core::General blocking requests.
Component: General → Plug-ins
QA Contact: general → plugins
So it looks like we're honoring the mimetype from the server, rather than the one of the type-attribute. Not actually sure what the right thing to do is. Biesi: do you know if this was an intentional change?
Assignee: nobody → jst
Flags: blocking1.9? → blocking1.9+
> So it looks like we're honoring the mimetype from the server, rather than the > one of the type-attribute. Right, that was one of the main effects of the fix for bug 1156.
Is that desirable even when there is an explicit type attribute?
Blocks: 1156
sicking: Yes. The TYPE attribute is just a hint, useful only for determining whether the DATA should be fetched. See bug 95549.
I guess the question is; how many pages depends on current behavior? If we're breaking lots of pages by following the spec, we should consider trying to get the spec changed, or simply ignoring the spec. This would of course be very unfortunate, but not unprecedented. I guess we will know a little better after the beta release.
Summary: Clicking "Watch the Keynote Address" does nothing → server provided content-type takes precedent over type-attribute
We already "fixed" one case in bug 389677, btw.
HTML5 prescribes certain behaviors in this case, but they're 1) apparently not final yet and 2) inconsistent (perhaps intentionally, but certainly not predictably). http://www.whatwg.org/specs/web-apps/current-work/#the-embed The <embed> element says that the type attribute takes precedence over the server-sent type. However, note the following, especially the second: > Big Issue: Should we instead say that the content-sniffing that we're going > to define for top-level browsing contexts should apply here? > Big Issue: Should we require the type attribute to match the server > information? http://www.whatwg.org/specs/web-apps/current-work/#the-object The <object> element says that the server-sent type takes precedence over the type attribute. However, note the following: > Big Issue: [The resource type algorithm for <object>] says to trust the > type. Should we instead use the same mechanism as for browsing contexts? > Big Issue: shouldn't we use the image-sniffing stuff here? [last step in > handling algorithm before going to fallback content] One last oddity I note here: if there's no type attribute or server-sent type <embed> does nothing, but <object> sniffs the downloaded resource.
Attached patch Proposed fix. (obsolete) — — Splinter Review
This fixes the apple site and the testcase. This makes us honor the given type attribute as long as it's a supported plugin type. I'm still afraid of leaving this per spec in Firefox 3, given how long Firefox has worked the way it did before we changed this, so IMO we should revert on this change.
Attachment #294980 - Flags: superreview?(bzbarsky)
Attachment #294980 - Flags: review?(cbiesinger)
I'm really not happy with that solution.... Apart from not honoring something that the spec is quite clear about, there's the problem where the proposed patch lets plug-ins override things that we could handle internally. For example, if a plug-in advertizes text/html, and the object has type="text/html" we'll ignore the server-provided type. If we feel we want to make a change like this, I think the right check is that GetTypeOfContent() == eType_Plugin, in addition to the IsSupportedPlugin() check here (the latter is needed to avoid the default plugin mess; another option would be a way to tell GetTypeOfContent to skip the default plugin check). But I'd really like to know what Opera and Safari do with this testcase....
http://biesi.damowmow.com/object/011.html - displays a broken-image icon in Safari and not FAIL - displays PASS in Opera The *Apple* page - works in Safari, of course :-P - does nothing in Opera when you click the link So we agree with Opera and disagree with Safari.
I filed <http://bugs.webkit.org/show_bug.cgi?id=16690> against Safari on this and noted they're violating the current HTML5 spec and the HTTP RFC. I also sent an email to whatwg about HTML5 not specifying the display of fallback content when the object's content is malformed (Safari's broken-image behavior instead of displaying the fallback content), although strictly speaking that discussion's for another bug if it's needed: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2007-December/013548.html
Comment on attachment 294980 [details] [diff] [review] Proposed fix. sr-, per comment 15.
Attachment #294980 - Flags: superreview?(bzbarsky) → superreview-
I still think we need to do something here. I'll be posting an updated patch that takes bz's suggestion into account.
Priority: P3 → P1
Target Milestone: --- → mozilla1.9beta4
Has anyone bothered raising this with either whatwg or public-html? Basically, we've made a serious change in the way we do <object> in general in 1.9. If we can't get away with the spec behavior while making the other changes, we probably can't get away with it at all, and the spec needs to change. But is it really true that we can't get away with it? How common is it for people to use <object> for Gecko? As I recall correctly, only pages that don't care at all about working in IE, or that are using JS to generate different content in different UAs are doing that, no?
The place I've seen break is apple.com and they are still producing new pages that break, the new macbook air site has broken videos because of this. As always, it's really hard to know how many pages are affected. But in general plugin stuff is very fragile and changing it scares me. What reason was there to make this change. The HTML4 spec I don't think has been an authoritative source in these matters for ages, and the HTML5 spec I think is mostly documenting what current browsers do.
It's the HTTP RFC, section 7.2.1: http://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html#sec7.2.1 "If and only if the media type is not given by a Content-Type field, the recipient MAY attempt to guess the media type via inspection of its content and/or the name extension(s) of the URI used to identify the resource." A type is given, and the spec doesn't leave room to ignore it.
The problem is that if current browsers are inconsistent (which they are), HTML5 will generally specify the thing that makes the most "sense" (in this case, following HTML4 and the HTTP RFC). So if we decide we can't do that, we need to raise it with the working group so that the spec reflects that. I do wonder how Apple's site works in IE... Do it send different markup? I sort of sympathize with your fears, but I think the right course of action it to evangelize apple to fix their site as a start. The fact that this hasn't come up much through the betas makes me thing that this is in fact a pretty rare problem...
Looks like apple is sending the same markup to everyone, but then use some AC.Quicktime.packageMovie library to muck around with the DOM to get it to work everywhere.
Attached patch Updated fix. (obsolete) — — Splinter Review
bz, I know you hate to see us do this, but I still don't think we can ship with this as things are, so here's an updated patch for your consideration.
Attachment #294980 - Attachment is obsolete: true
Attachment #304642 - Flags: superreview?(bzbarsky)
Attachment #304642 - Flags: review?(cbiesinger)
Attachment #294980 - Flags: review?(cbiesinger)
Safari just fixed their misbehavior here. I think we should wait and see what they're willing to break before declaring failure here; solidarity can make this stick.
Does safaris change break apple.com? That was one of the sites the broke the most.
Have we contacted Apple about this? I'd think that we should try evangelism before we try to bail on things here altogether...
I personally think apple.com is only one of many. It's the unknown ones that scare me, more so than the ones we know about.
Do you have numbers, or just anecdotes of things breaking? If just anecdotes, then why doesn't this bug have any dependencies or duplicates? This bug has no duplicates, nor do I see any duplicates in dependencies of bug 1156, so we have no record that anything else has ever broken. Further, how can anyone possibly refute that argument? It isn't even an argument, just fear. Safari still handles Apple keynote, but it isn't (and never has been) due to ignoring server content types. They're handling the outermost of the <object>s, which seems to specify an ActiveX control (!?), which we ignore entirely. The <object> inside it is the one we handle, which has a GIF image as data but video/quicktime as type. Both latest Safari trunk and Safari 3 have the exact same behavior on the inner <object>, displayed alone: don't display the image, but display a little Quicktime icon (generated by Quicktime -- I have no idea why our plugin displays the GIF but Safari's displays the icon). This is what we have the Quicktime plugin render, if we ignore Content-Type (Safari has Quicktime generate a little icon): data:text/html,%3Cobject%20type=%22video/quicktime%22%20data=%22http://images.apple.com/quicktime/qtv/mwsf08/images/hero_btn_watch20080115a.gif%22%20id=%22movieInner%22%20width=%22231%22%20height=%2223%22%3E%3Cparam%20value=%22http://stream.qtv.apple.com/events/jan/f27853y2/m_972345688g_650_ref.mov%22%20name=%22href%22%3E%3Cparam%20value=%22quicktimeplayer%22%20name=%22target%22%3E%3Cparam%20value=%22true%22%20name=%22autoplay%22%3E%3Cparam%20value=%22false%22%20name=%22controller%22%3E%3Cparam%20value=%22true%22%20name=%22cache%22%3E%3Cparam%20value=%22#000000%22%20name=%22bgcolor%22%3E%3Cparam%20value=%22http://images.apple.com/quicktime/qtv/mwsf08/images/hero_btn_watch20080115a.gif%22%20name=%22posterframe%22%3E%3Cparam%20value=%22true%22%20name=%22saveembedtags%22%3E%3C/object%3E This is what we show as blank (Safari renders a clickable button): data:text/html,%3Cobject%20id=%22movie%22%20codebase=%22http://www.apple.com/qtactivex/qtplugin.cab#version=7,3,0,0%22%20width=%22250%22%20height=%2223%22%20classid=%22clsid:02BF25D5-8C17-4B23-BC80-D3488ABDDC6B%22%3E%3Cparam%20value=%22http://images.apple.com/quicktime/qtv/mwsf08/images/hero_btn_watch20080115b.gif%22%20name=%22src%22%3E%3Cparam%20value=%22http://stream.qtv.apple.com/events/jan/f27853y2/mw2008_hd.qtl%22%20name=%22href%22%3E%3Cparam%20value=%22quicktimeplayer%22%20name=%22target%22%3E%3Cparam%20value=%22true%22%20name=%22autoplay%22%3E%3Cparam%20value=%22false%22%20name=%22controller%22%3E%3Cparam%20value=%22true%22%20name=%22cache%22%3E%3Cparam%20value=%22#000000%22%20name=%22bgcolor%22%3E%3Cparam%20value=%22http://images.apple.com/quicktime/qtv/mwsf08/images/hero_btn_watch20080115b.gif%22%20name=%22posterframe%22%3E%3Cparam%20value=%22true%22%20name=%22saveembedtags%22%3E%3C/object%3E Also, I should note the problem is that *that specific page* is passing a non-video/quicktime URL as the second parameter of |AC.Quicktime.packageMovie|, in inline JavaScript. This isn't a library problem, it's a problem specific to one single page -- note for example that <http://www.apple.com/quicktime/qtv/mwsf07/> doesn't have this problem (although it's using a different library to do its stuff). I have a detailed description of the problem and diagnosis written up which I'm trying to submit to Apple -- just need to determine what's the right channel to see it gets visibility. bugreport.a.c doesn't offer an option for Apple websites. By the way, there's a simpler reference for correctness than RFC 2616, which oddly I haven't seen mentioned here or elsewhere: the HTML spec for <object> and the type attribute is *very* clear about what should happen. http://www.w3.org/TR/html401/struct/objects.html#adef-type-OBJECT
I greatly fear <http://www.apple.com/contact/feedback.html> is a black hole for this sort of detailed feedback, but better that it get submitted than that it not get submitted due to this fear.
(In reply to comment #33) > This bug has no duplicates... This bug has three duplicates so far. Are those not the same issue?
> They're handling the outermost of the <object>s Ugh. And yes, there are three duplicates for this bug: Bug 414310: <object> says audio/x-mplayer2, server reports audio/midi, file is midi, plug-in supports midi but doesn't claim to support it. This page apparently uses script to create the exact object tag, which is why it's not running into the compat issues you'd think. Bug 416480: Flash being served as image/swf. Original site is password-protected, so can't tell how it's set up. Bug 416867: Flash being served as application/octet. If they just served application/octet-stream, it would all work... Seems to generate the <object> in JS somehow.
Comment on attachment 304642 [details] [diff] [review] Updated fix. >+++ b/content/base/src/nsObjectLoadingContent.cpp >+ GetTypeOfContent(mContentType) == eType_Plugin) { > mContentType = channelType; That should be a != test, no? Otherwise, consider a plug-in that advertises text/html, with a type="text/html" and a server-provided type of image/gif. IsSupportedPlugin(mContentType) will be true, GetTypeOfContent(mContentType) will be eType_Document, so we'll fail this test, end up in the else, and smack the channel to be text/html. With that test fixed, I suppose I can live with this, though I would really really prefer that we just made more of an outreach effort to sites. It seems like if we can't ship this change now, we'll never be able to ship it...
Attachment #304642 - Flags: superreview?(bzbarsky) → superreview+
(In reply to comment #33) > Do you have numbers, or just anecdotes of things breaking? If just anecdotes, > then why doesn't this bug have any dependencies or duplicates? This bug has no > duplicates, nor do I see any duplicates in dependencies of bug 1156, so we have > no record that anything else has ever broken. No numbers, just fear having seen many things like this go bad throughout the years. And this bug does have a couple of duplicates (not many), search in the bug and you'll find them.
(In reply to comment #35) > This bug has three duplicates so far. Are those not the same issue? (In reply to comment #36) > > They're handling the outermost of the <object>s This is what I get for commenting too late at night.
Frankly, if we're setting the barrier for reverting a change made years ago at three duplicates + fear, I question our ability to make nearly any changes to better conform to standards.
I got into trouble (Bug 414310) with <object> because <embed> is non-standard according to HTML 4.01. I avoided it. The rule in HTML 5 that allows the server-provided type to have precedence assumes <embed> will be the preferred tag, nested inside the <object>, due to <object> compatibility issues (big thank you to Microsoft). Don't some plug-ins work better with <object> -- they're more scriptable that way? Just because it's there, doesn't make <embed> good. It seems second-rate.
So here's my opinion on the matter: Why do we want the server content-type to take precedence over the attribute? I consider the HTML4 spec to be largely irrelevant on matter so that should IMO not be the reason. The HTML5 spec I think mostly follows what browsers do. Seems weird if we're chasing them and they're chasing us. So again, that in and of itself should IMO not be the reason. Is there a semantic or practical reason why the server should win? Does it make it easier to develop sites? I'm asking this seriously, not advocating either way. Just asking why people are pushing one way or the other. Letting the attribute take precedence *does* let you do what apple is doing, that is, choose the plugin independent of the type of data on the server. Could be useful if multiple plugins can handle the same format.
> Is there a semantic or practical reason why the server should win? It makes the type information attached to the data as opposed to being in a different place altogether. Assuming a semi-competent server admin, this makes it much harder for the metadata and the data to get out of sync. Of course the flip side is that if the server admin is not competent or the site author doesn't realize they need to send the right MIME types with their data you get issues. Looked at another way... why do we prefer the server-provided MIME type to our own sniffing for toplevel documents or iframes? If we want to allow selecting a specific plug-in implementation, we should do it via the existing classid mechanism (which is expressly designed for that) instead of overloading @type like this, in my opinion.
As described in comment 13, the HTML5 spec follows what is currently implemented in Firefox 3, Opera, and Safari trunk; given the toeing the line on sniffing that partly motivates the HTML5 position, I don't see how this differs. Respecting the type attribute over the metadata provided by the server essentially removes control over the interpretation of the content of a resource from the site serving it. Any malicious operator who can upload content to your server, mislabeled and misnamed though it might be, can cause that content to be interpreted arbitrarily. Flash bases security off the location of the Flash content, so now you have a mistyped not-image which can run code on your site and communicate interesting data offsite. (I'm basing this off <http://www.adobe.com/devnet/flashplayer/articles/secure_swf_apps_07.html>, which I have not read before now, so it's possible I don't have all the details exactly right.)
That brings up a good point. What does other UAs do? IE is notorious for ignoring server-sent content-types.
Last I checked, IE only looked at <object>s which had a classid="clsid:..." (the ones we ignore).
Blocks: 65092, 95549
WebKit handles the outer object due to the following hard-coded list of clsids, apparently: http://trac.webkit.org/projects/webkit/browser/trunk/WebCore/rendering/RenderPartObject.cpp#L77
Would people be happier if we left our type detection alone and added a similar hack to deal with this issue? I don't think I would...
I would. If *this issue* is what's motivating this bug, then a surgical strike seems appropriate. Furthermore, as bz has pointed out, the Right Way to pick a specific implementation regardless of the content type is via the classid attribute. This solution is only a "hack" in that there is no general framework in place to handle arbitrary classids.
(In reply to comment #22) > As always, it's really hard to know how many pages are affected. I believe I can answer that. I ran a MapReduce over Google's cache of the current web, looking for pages with [OBJECT TYPE="..." DATA="..."] markup, and then cross-referenced it with the HTTP headers of the linked media files. The percentage of [OBJECT] elements whose TYPE attribute doesn't match the linked Content-Type is about 2%. This is consistent across several sample sizes. The most common error was TYPE="application/x-shockwave-flash" but Content-Type: application/octet-stream. The second most common error was TYPE="application/x-shockwave-flash" but Content-Type: application/-shockwave-flash2-preview. In fact, these two errors overwhelmed the results; all other errors combined were a distant third. Note: my sample may be biased, because it only included pages that included OBJECT markup directly; it did not include pages (like the Apple.com pages) that dynamically generate OBJECT elements via script.
Er, that should read: The second most common error was TYPE="application/x-shockwave-flash" but Content-Type: application/x-shockwave-flash2-preview. With an "x-".
Mark, we ignore the server-sent type for <object> if it's application/octet-stream and there is a type attribute. If you ignore that case (in which behavior is unchanged), what fraction of pages still have a problem? I do think that pages that dynamically generate the <object> are more likely to have issues, since they don't necessarily worry about IE compat and hence might be using the nested <object> setups IE used to not support, etc. jst, I'd almost be happier hardcoding those few clsids (assuming we're pretty sure that the activex and npapi plug-ins there have equivalent functionality) than doing the content-type thing.
application/octet-stream accounts for about half of the mismatches. If you're already ignoring that, then the percentage of potentially problematic pages drops to almost exactly 1%.
I don't know about this. If you took over the same classid IE uses there would be no end of problems, for WMP alone. The first would be that there are types in the IE object tag the plugin doesn't handle like application/x-oleobject. Another is scripting to the WMP controls. Take <object id="MediaPlayer" type="application/x-mplayer2" height="64" width="240"> To be compatible you would have to change almost every script: function play() {document.MediaPlayer.controls.play()} to function play() {document.getElementById('MediaPlayer').controls.play()} You'd at least need a different kind of classid so the non-IE variant or the IE variant can continue to be ignored by the other.
(In reply to comment #51) > Er, that should read: > > The second most common error was TYPE="application/x-shockwave-flash" but > Content-Type: application/x-shockwave-flash2-preview. > > With an "x-". So what would the current code do with this? Does flash register as handler for "application/x-shockwave-flash2-preview"? If nothing is register as handler, do we fall back to using the attribute? Would it make sense to do that?
Tom, no one is suggesting mapping the WMP classid to a MIME type. The classids we're talking about are the ones pointed to in comment 47. No one is suggesting we map all classids to MIME types. > Does flash register as handler for "application/x-shockwave-flash2-preview"? At least on Linux, with some Flash versions, doesn't seem to. > If nothing is register as handler, do we fall back to using the attribute? Not right now. > Would it make sense to do that? Maybe... It's really not that happy in some ways, but if too much stuff would break... Mark, did you exclude <object>s with classid="" from your mapreduce? Or are they included?
My MapReduce did nothing whatsoever to include or exclude object elements with the classid attribute. If it would make a difference in your decision, I could probably run another MapReduce with more specific parameters. Talk amongst yourselves about exactly what combination of parameters would be most helpful, and I'll see what I can do. (If it wouldn't make a difference in your decision, I'd rather not run another. They're somewhat resource-intensive.)
(In reply to comment #56) > > If nothing is register as handler, do we fall back to using the attribute? > > Not right now. > > > Would it make sense to do that? > > Maybe... It's really not that happy in some ways, but if too much stuff would > break... Why would this be a bad idea. Isn't that sort of the idea of the type-attribute, that it's a hint. And if the server isn't providing us with any useful information then using the hint seems reasonable. > Mark, did you exclude <object>s with classid="" from your mapreduce? Or are > they included? It would certainly be an interesting solution to have a small list of "supported" classid, like what safari does. Would that really be less evil than falling back to the attribute.
(In reply to comment #58) > (In reply to comment #56) > > > If nothing is register as handler, do we fall back to using the attribute? > > > > Not right now. > > > > > Would it make sense to do that? > > > > Maybe... It's really not that happy in some ways, but if too much stuff would > > break... > > Why would this be a bad idea. Isn't that sort of the idea of the > type-attribute, that it's a hint. And if the server isn't providing us with any > useful information then using the hint seems reasonable. Are you proposing maintaining a list of known bogus types which, if sent from the server, should be ignored in favor of an alternative (if one exists)?
How about hard-coding application/x-shockwave-flash2-preview --> application/x-shockwave-flash ? That seems like the smallest change to improve compatibility with that last 1%.
> Are you proposing maintaining a list of known bogus types which, if sent from > the server, should be ignored in favor of an alternative (if one exists)? No, I'm suggesting that we try to use the server-provided content type, but if there are no currently registered plugins for that content-type, try to use the type specified in the attribute.
s/plugins/handlers/ in comment 61, I assume? E.g. if the server sends text/html, we should just render it as HTML. Mark, the classid check is the only change that would be needed to the MapReduce you ran. If there _is_ a classid then we ignore the <object> altogether, so it doesn't matter what the type attribute and the server type are. If there isn't a classid, then we do care about the server type in our current implementation. Comment 60 might also be a good idea...
Don't think this blocks b4, if it requires beta exposure please leave as P1, otherwise move to P2.
Target Milestone: mozilla1.9beta4 → mozilla1.9
Flags: tracking1.9+ → blocking1.9+
Priority: P1 → P2
The more I think about this the more I still think we should just revert to the way we used to do this. Hardcoding mimetype mappings or classid to mimetype mappings really isn't something I want to do any time other than really early in a release cycle, certainly not now.
Comment on attachment 304642 [details] [diff] [review] Updated fix. Sicking, this needs the == changed to an != in the GetTypeOfContent(mContentType) == eType_Plugin check, other than that it's good to go.
Attachment #304642 - Flags: review?(cbiesinger) → review?(jonas)
Can we get a bug filed to investigate this early in the next cycle and come up with a better fix if there is one?
bz: Jonas had a look at this and we ended up talking through this and realized that the patch as it's written (with the == check changed to !=) won't always do the right thing. The case where it fails is when there is no type attribute, i.e. mContentType is empty, and the default plugin is enabled. The code looks like this: if ((mContentType.IsEmpty() || !channelType.EqualsASCII(APPLICATION_OCTET_STREAM)) && !IsSupportedPlugin(mContentType) && GetTypeOfContent(mContentType) != eType_Plugin) { mContentType = channelType; } else { chan->SetContentType(mContentType); } With mContentType empty, we'll fall into the else case because GetTypeOfContent() will return eType_Plugin *if* the default plugin is enabled, which will mean we don't pass the stream type or anything useful to the default plugin. I personally don't see why this test isn't simply just !IsSupportedPlugin(mContentType). Seems to me that if mContentType here is text/html, and there's a plugin installed that does support text/html, we'll still later on call GetTypeOfContent(mContentType), and that'll return eType_Document and we'll display the HTML w/o actually using the plugin (not that I care at all whether we or the plugin displays the HTML in such a case). That's what the old code did, instantiated a plugin based on the type information available in the HTML, if it was a supported type, if not, kick off the request and try again based on the channel type. This would mimic that exactly wouldn't it?
> Seems to me that if mContentType here is text/html, and there's a plugin > installed that does support text/html The problem is that if we have <object type="text/html"> and the data is a GIF, we'll handle it as GIF if the plug-in is not installed and as HTML otherwise. We don't want that behavior, in my opinion. > This would mimic that exactly wouldn't it? Yes, but I don't think we want to mimic the old behavior exactly... Can we just ignore the GetTypeOfContent() return when mContentType is empty? It seems like in that case we'd want to handle based on the channel type no matter what.
(In reply to comment #68) > > Seems to me that if mContentType here is text/html, and there's a plugin > > installed that does support text/html > > The problem is that if we have <object type="text/html"> and the data is a > GIF, we'll handle it as GIF if the plug-in is not installed and as HTML > otherwise. We don't want that behavior, in my opinion. So admittedly I don't care super much about this situation. It seems highly theoretical. But seems easy enough to deal with using the following expression: if (!IsSupportedPlugin(mContentType) && GetTypeOfContent(mContentType) != eType_Document && GetTypeOfContent(mContentType) != eType_Image) { mContentType = channelType; } else { chan->SetContentType(mContentType); }
Let's step back for a second. We want to use the channel type unless one of the following is true: 1) The channel type is application/octet-stream and we have a type hint 2) Our type hint is a type that we support with a plugin. The correct condition to express that is: (channelType.EqualsASCII(APPLICATION_OCTET_STREAM) && !mContentType.IsEmpty()) || (IsSupportedPlugin(mContentType) && GetTypeOfContent(mContentType) == eType_Plugin) Since the above scenario is actually the "else" condition of the if, it looks like the right code here is: if ((mContentType.IsEmpty() || !channelType(EqualsASCII(APPLICATION_OCTET_STREAM)) && (!IsSupportedPlugin(mContentType) || GetTypeOfContent(mContentType) != eType_Plugin)) { mContentType = channelType; } else { chan->SetContentType(mContentType); } Now if mContentType is empty, !IsSupportedPlugin(mContentType) will be true, as will the IsEmpty() check so we'll take the first branch. I guess the difference with my original comments on the patch is that we need to use the '||' there in addition to negating the GetTypeOfContent() condition. Maybe we should flip the if/else cases here and use the first boolean expression from this comment?
I'm not sure I agree with "support as a plugin" in your list. Why not simply "support". That gives the list Use server content type unless 1) The channel type is application/octet-stream and we have a type hint or 2) Our type hint is a type that we support. But the first condition doesn't make sense to me. If we have a type hint and it's a supported one then rule 2 applies. If we have a type hint and it's not supported, or if we don't have a type hint, then always using the server type seems like the right thing to do. If the server type is one that we don't support (such as application/octet-stream) doesn't really matter here since we're not going to be able to do anything useful anyway. Or are you concerned about the case when both the attribute and server types are ones that we don't support, which one we pick to use in the plugin finder or whatnot?
> 2) Our type hint is a type that we support. Because we really should be honoring the server type in all the cases where it's remotely feasible. Therefore, I would like to restrict the cases where we don't honor it as much as possible. > If we have a type hint and it's a supported one then rule 2 applies. Yes... > If the server type is one that we don't support (such as application/octet- > stream) doesn't really matter here since we're not going to be able to do > anything useful anyway. Sure we will. If the server type is octet-stream and the type hint is something sensible but not something we support, we can prompt the user with the right plugin-install stuff. > which one we pick to use in the plugin finder or whatnot? Clearly I should read all the way through before responding? ;) octet-stream is the one guaranteed-useless type and sadly common, it seems, so we're basically ignoring it for object. It's also the type that text/plain sniffed as binary will become in this situation, and there we also want to just go with the type hint. Given those considerations we really do want condition 1. And I would like to restrict condition 2 as narrowly as we can while allaying jst's concerns about breaking the web. Once we implement something more like safari's classid mapping (which I think we should) I think we should drop condition 2 altogether.
Attached patch Updated fix. — — Splinter Review
Boris, this is directly out of your earlier comment in the bug, looks more understandable to me.
Attachment #304642 - Attachment is obsolete: true
Attachment #310107 - Flags: superreview?(bzbarsky)
Attachment #310107 - Flags: review?(jonas)
Attachment #304642 - Flags: review?(jonas)
Comment on attachment 310107 [details] [diff] [review] Updated fix. So i still think we should do the simple thing here and always use the attribute if it's something we support, be it html or a plugin. But i'm ok with this too I guess.
Attachment #310107 - Flags: review?(jonas) → review+
Comment on attachment 310107 [details] [diff] [review] Updated fix. Looks good. Thanks for putting up with me, Johnny!
Attachment #310107 - Flags: superreview?(bzbarsky) → superreview+
Fixed!
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
What's the bug number of the follow-up bug from comment 66?
Embedding plugins in generated HTML offline? Http protocol works fine. Last time I downloaded a nightly build, a few weeks ago, offline also still worked. I feel fairly foolish for asking this.
that sounds like something completely different from this bug. Please file a separate bug and attach a testcase.
An object or an embed tag when there is no server (file:// protocol windows) causes an error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "<unknown>" data: no] I'm guessing this is the same bug, because when you don't specify type in the tag the error goes away. Playing works.
I bet that's unrelated to this bug, more likely bug 428600 (or a close cousin to that bug at least).
When loading from file://, the type attribute is authoritative. So yes, that's bug 428600 or similar.
> What's the bug number of the follow-up bug from comment 66? Bug 444872.
Since the betas of FF3 (Windows), hoping it might get fixed: Plugins that are embedded in generated file:// loaded pages don't appear and don't play. Is it this bug -- because tags and content type are handled differently and some double-checking offline and generated code fails? Should I file as a new bug with an example? If disabling generated plugins is a security measure, is there a setting to switch it off somehow?
That doesn't sound like this bug. If you have an example showing the problem, please file a new bug and cc me on it.
Done. Thanks. Bug 445004.
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: