<object> tag alt content is not displayed if both 'type' and 'data' attributes are absent

VERIFIED FIXED in mozilla0.9.3

Status

()

Core
Layout
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: Andriy Palamarchuk, Assigned: av (gone))

Tracking

Trunk
mozilla0.9.3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

17 years ago
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux i586; en-US; rv:0.9.2+) Gecko/20010709
BuildID:    2001070921

When I go to the URL I see only black screen.

Reproducible: Always
Steps to Reproduce:
Open the URL in browser.

Shows Flash animation when I open the URL with Konqueror

Comment 1

17 years ago
WFM, linux 2001070909 without flash plugin. 0.9.2 with flash crashes.
Same problem is occurring on 2001070604 on Windows NT, will test on a newer 
build when I get a chance...

WFM in IE 5.5, 4.77 with flash and with 2001070604 without flash.

Updating OS to all, summary to include the URI
OS: Linux → All
Summary: Can't access the site → Can't load http://www.blueflameinc.com/

Comment 3

17 years ago
Also confirming in build 2001-07-10-04, Win2k sp2

I don't crash, I get a black screen.

Someone needs to NEWify this bug.  :)

Comment 4

17 years ago
NEWified as requested ;)
Status: UNCONFIRMED → NEW
Component: Evangelism → Plug-ins
Ever confirmed: true
Keywords: flash

Comment 5

17 years ago
Not evang -> default owner of Plugins
Assignee: bclary → av
QA Contact: zach → shrir

Comment 6

17 years ago
Created attachment 42053 [details]
simplified testcase

Comment 7

17 years ago
The problem seems to be with this simple combination (as demonstrated in the
attached testcase:)

<object>
<embed src="..." type="...">
</object>
Haven't looked at the frame model or in the debugger yet.

Andrei, isn't this pretty common?
Component: Plug-ins → Layout
Hardware: PC → All
Summary: Can't load http://www.blueflameinc.com/ → Plugin doesn't render: problem with <object><embed></object>
(Assignee)

Comment 8

17 years ago
I looked at it yesterday and it seems to try to go the <object> path. The mime 
type is not specified and we cannot instantiate the plugin but it still returns 
NS_OK thus, not going to CantRenderReplacedElement() at the bottom of 
nsObjectFrame::Reflow() and therefore not going to the <embed> tag which would 
work otherwise. I am still not sure if this is just bad html or we should fix it 
on our side.
(Assignee)

Comment 9

17 years ago
Peter, remember some quite long time ago you fixed the bug when if there is no 
mime type specified and we cannot figure it out we still open the stream and see 
if the server feeds us with it. Here what happens in this bug: no mime type, we 
start to load, server says that mime type is 'text/html', in onStartRequest we 
try innstantiate the plugin, fail _and_ totally ignore this fact.

Trying to find out what we can do here.
Status: NEW → ASSIGNED

Comment 10

17 years ago
But the type= attribute on the EMBED tag should override that and in fact, that
code-path shouldn't even happen. I believe it should only happen when we can't
determine the mine type by the extension or any other way. Is the OBJECT tag
triggering this code path? How can that be without a data= attribute? Maybe that
code changed with recent networking changes.
(Assignee)

Comment 11

17 years ago
OK, I got it. The logic at the end of nsObjectFrame::Reflow method is wrong. 
What it does (in pseudo-code)

  if(!mimetype && data) {
    try to get the mime type;
  }

  rv = InstantiatePlugin();

  if(NS_FAILED(rv))
    go for alt content;

Couple of things are wrong here:
1. InstantePlugin should be in different scope
2. we use nsString thing !src.get() to detrmine if the data attribute is present 
which is wrong, it should be src.Length() != 0 instead

So, logicwise I beleive it should look like:

  if(!mimetype && !data) {
    rv = error;
  }
  else {
    if(!mimetype) {
      try to get the mime type;
    }
    rv = InstantiatePlugin();
  }

  if(NS_FAILED(rv))
    go for alt content;

The patch is coming.
(Assignee)

Comment 12

17 years ago
Created attachment 42133 [details] [diff] [review]
first try -- corrected logic in instantiating plugin
(Assignee)

Comment 13

17 years ago
Rephrasing summary, setting milestone and keyword.
Keywords: correctness
Summary: Plugin doesn't render: problem with <object><embed></object> → <object> tag alt content is not displayed if both 'type' and 'data' attributes are absent
Target Milestone: --- → mozilla0.9.3
(Assignee)

Updated

17 years ago
Whiteboard: [SEEKING REVIEW]
This may also fix bug 65455.

Comment 15

17 years ago
r=peterl

Comment 16

17 years ago
Would it be equivalent to say...

  const char* mimeType = mimeTypeStr.get();
  if (!mimeType && src.Length() > 0) {
    /* Try to set mimeType from extension */
  }

  if (mimeType)
    rv = InstantiatePlugin(/*...*/);
  else
    rv = NS_ERROR_FAILURE;

If so, that'd seem a bit cleaner to me...
(Assignee)

Comment 17

17 years ago
This is not equivalent, in my version I still do InstantiatePlugin() even if we 
failed to get a mime type from and existing extension. It will check if it can 
get the mime type somewhere later.

Comment 18

17 years ago
I see. How about reversing the logic so that the positive case comes first, 
then? e.g.,

  if (mimeType || src.Length() > 0) {
    if (!mimeType) {
      /* try to infer from src attribute */
    }

    rv = InstantiatePlugin(/* ... */);
  }
  else
    rv = NS_ERROR_FAILURE; // no MIME type, no src.

Also, it would be nice to note that even if |src| inference fails, we may be 
able to determine the appropriate type later.
(Assignee)

Comment 19

17 years ago
Created attachment 43437 [details] [diff] [review]
second try -- easier to read layout as per waterson's comment

Comment 20

17 years ago
thanks -- sr=waterson
(Assignee)

Comment 21

17 years ago
Checked in to the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
Whiteboard: [SEEKING REVIEW]

Comment 22

17 years ago
verif on trunk, all plfs, 1113, url works now
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.