196735 - <OBJECT src=...> should be ignored

Status: VERIFIED FIXED

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

Description

[Malcolm Rowe]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130
(guessed at correct component)

The <object> element in HTML4 has never supported a SRC attribute, but for some
reason we allow SRC to be an alias for DATA (perhaps we share a common
implementation between <object> and the proprietry <embed> tag?).

<object src=...> works, but it shouldn't. More importantly, if an object is
declared with both SRC and DATA attributes, SRC takes priority.

See the testcase (to follow).

Comment 1

[Malcolm Rowe]

Attachment: Testcase showing that SRC= overrides DATA= on OBJECT

Comment 2

[Malcolm Rowe]

[If this bug is fixed, then this comment is no longer relevant, but I'll note it
here in case the bug gets WONTFIXED.]

Probably because they're all expected a DATA attribute for images, the context
menu, image property dialog and page info all experience problems with this
testcase, though none of them are serious.

Comment 3

[Peter Lubczynski]

Looks like we always check SRC first here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/html/base/src/nsImageFrame.cpp#318

We should probably only do this in quriks mode.

The context menu is another problem.

Comment 4

[Boris Zbarsky [:bzbarsky]]

We MUST check "src" first for <img> elements, which is the common case for
nsImageFrame.

Once bug 83774 lands, though, the <object>/<embed> code will be in the object
frame and at that point the order can be switched, since the code will not be
affecting the <img> elements.

Comment 5

[Malcolm Rowe]

Just a quick note.

Looking at the LXR reference in comment 3, AFAICS, it says:
 get SRC
 if we didn't find it and we're OBJECT, get DATA
 use whatever we found to create an image

Why can't we change that logic to this:
 if we're OBJECT, get DATA
 else, get SRC
 use whatever we found to create an image

Would that cause problems with EMBED?

Is there a reason this change might need to be quirks-only?  IE6 already 
works 'correctly' with regard to this behaviour (in both strict and non-strict 
modes), although it doesn't seem to support image/png in OBJECT.

I'm going to attach a new testcase that works in IE6 to demonstrate this, 
though I'm afraid I can't test in Mozilla at the moment.

Comment 6

[Malcolm Rowe]

Attachment: Testcase #2 (works in IE6)

Comment 7

[Malcolm Rowe]

Huh, that second testcase only works locally for me. I think IE must be 
blocking access to the images because they're in a different domain (or 
something).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Yeah, that change would work.

Again, this code is about to all go away completely, so I wouldn't bother
changing it right now...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note the following hunks from the bug 83774 patch, btw (in nsObjectFrame.cpp):

+    nsAutoString data;
+    rv = aContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::data, data);
+    if (rv != NS_CONTENT_ATTR_HAS_VALUE) {
+      rv = aContent->GetAttr(kNameSpaceID_None, nsHTMLAtoms::src, data);
+    }
+    NS_ASSERTION(rv == NS_CONTENT_ATTR_HAS_VALUE,
+                 "Why did IsSupportedImage() return PR_TRUE?");
+    imageLoader->ImageURIChanged(data);

So that patch should in fact fix this bug....

Comment 10

[Malcolm Rowe]

Ok, though bug 83774 is targetted for 1.5, and this could easily go in before
that, but I see your point.

Is there any reason you can't remove the check for the SRC attribute entirely
from nsObjectFrame [in bug 83774's patch]?

The reason that I originally filed this bug was because I wrote a load of XHTML
using OBJECT SRC= (naturally, thinking along the lines of IMG SRC=), which
seemed to work fine. I was unpleasantly surprised when it didn't validate. DATA
overriding SRC was just something I noticed at the same time.

Since IE doesn't really support OBJECT yet, there doesn't seem to be any
backwards-compatible argument for allowing OBJECT SRC= to work.  Can we ditch it
entirely?

Comment 11

[Boris Zbarsky [:bzbarsky]]

IE supports <object> just fine last I checked....

As for the compat issues, I don't know.  Peter?  Does anyone actually use
<object src="">?  (Note that the nsObjectFrame code is used for <embed> too,
btw, and <embed> _does_ use src; but we could condition on the tagname.... or
better yet finish the job of moving that stuff into the content node).

Comment 12

[Malcolm Rowe]

Apologies - my comment about IE not supporting OBJECT well more to do with being
able to use it as a replacement of the IMG tag (which it's *not* very good at -
the first testcase above doesn't work in IE6, despite the fact that IE [sort-of]
supports PNG images).

How would I go about testing EMBED? Any pointers to a 'spec' for it?

Comment 13

[Boris Zbarsky [:bzbarsky]]

http://www.blooberry.com/indexdot/html/tagpages/e/embed.htm pretty much sums it up.

Comment 14

[Malcolm Rowe]

Attachment: Prevent SRC attribute from being considered for image OBJECTs

The attached patch stops us from checking the SRC attribute when we encounter
an OBJECT that is rendering an image. This brings us into line with our
behaviour for IMG, EMBED, and non-image OBJECT elements.

I've tested this as thoroughly as I can, using a variety of IMG, EMBED, APPLET,
and OBJECT tags, and there appear to be no unexpected side-effects.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117532 [details] [diff] [review]
Prevent SRC attribute from being considered for image OBJECTs

This should use an nsCOMPtr for the tag.. (yes, the existing code is really old
and crufty).

Comment 16

[Malcolm Rowe]

Attachment: Prevent SRC attribute from being considered for image OBJECTs (v2)

Updated to use nsCOMPtr for the tag.

Comment 17

[Malcolm Rowe]

-> me

Comment 18

[Malcolm Rowe]

dang bugzilla, ->me for real

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 117534 [details] [diff] [review]
Prevent SRC attribute from being considered for image OBJECTs (v2)

Looks good, if you put a space between the "if" and the '('....  r+sr=bzbarsky

Would you mind holding off on this till Saturday?  If bug 83774 hasn't landed
by then, ask someone to check it in?  (I'm still hoping to get it landed before
then, but if it's not in by then, it's not in for 1.4, so...)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Hrm.  See also
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsObjectFrame.cpp#476
and
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsObjectFrame.cpp#1124
(this one checks src before data) and
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsObjectFrame.cpp#3055

Looks like we can't ignore src="" in general (based on that last link); do we
want to do it just for images?

Comment 21

[Malcolm Rowe]

re: Comment 20
After a brief (5 minute) look at the code, I think that:

1. The check for SRC in IsSupportedImage should probably be removed.
2. I need to work out what runs this codepath - it might need a conditional
check for the OBJECT tag.
3. I think this is ok. bug 152334 comment 10 indicates that we're not parsing
the SRC attribute, we're fabricating a SRC PARAM based on the DATA attribute, if
there is no SRC attribute. But obviously, that does imply that there used to be
users of OBJECT src= for WMP or Real plugins.  I'll need to test that OBJECT
src= still works (it should just get reflected into a PARAM).

Comment 22

[Boris Zbarsky [:bzbarsky]]

Yeah, those were my impressions too.....

Comment 23

[Malcolm Rowe]

I won't get to this until tonight. If bug 83774 makes it into 1.4alpha, I'll 
reverify with the tests I'm using, otherwise I'll make a patch that can go into 
1.4beta.

Comment 24

[Malcolm Rowe]

bz, should this check in the second patch:
if (tag == nsHTMLAtoms::object)

actually be:
if (tag.get() == nsHTMLAtoms::object)

Just like in 
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsObjectFrame.cpp#6
21

Or does it not matter?  I'm not that familiar with nsCOMPtr, I'm afraid.

Comment 25

[Boris Zbarsky [:bzbarsky]]

bug 83774 won't fix the other places I pointed out in nsObjectFrame, so no
matter what there'll be something to do here.  ;)

As for .get(), you used to need it on nsCOMPtr when comparing to a pointer.  So
some old code has it.  Nowadays, you no longer need it and the recommended usage
is to not have it.

Comment 26

[Malcolm Rowe]

So, bug 83774 has now landed and stabilised, and things have changed slightly.
Although OBJECT data= doesn't override OBJECT src= anymore, OBJECT src= is still
accepted. EMBED data= now overrides EMBED src=, and EMBED src= is accepted where
it wasn't before.

The patch (to follow) makes EMBED function the way it did in 1.3 wrt SRC and
DATA attributes, and removes SRC from consideration for OBJECT (excluding the
backward-compatibility fudge that bz identified (see comment 21, point 3).

Comment 27

[Malcolm Rowe]

Attachment: Fix EMBED, prevent SRC attribute from being considered for OBJECT

Note: I'm not sure whether the changes to Reflow() [last hunk] are necessary -
I couldn't identify any differences in behaviour - but they seem helpful for
consistency.

Comment 28

[Malcolm Rowe]

Comment on attachment 118241 [details] [diff] [review]
Fix EMBED, prevent SRC attribute from being considered for OBJECT

Requesting r/sr from elsewhere, since bz is away.

Comment 29

[Peter Lubczynski]

Comment on attachment 118241 [details] [diff] [review]
Fix EMBED, prevent SRC attribute from being considered for OBJECT

r=peterl

Comment 30

[Malcolm Rowe]

Attachment: Better testcase

Since this bug morphed slightly after the checkin to bug 83774, I'm attached a
testcase that actually allows someone to test the checkin.  The testcase only
uses images for simplicity - I actually tested my changes with applets and
embedded 'objects' as well.

Comment 31

[Malcolm Rowe]

timeless was kind enough to check this in for me.

Marking FIXED.

Comment 32

[bmartin]

Marking Verified Fixed based on results from testcase in comment #30