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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Andriy Palamarchuk]

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

[Diego Biurrun]

WFM, linux 2001070909 without flash plugin. 0.9.2 with flash crashes.

Comment 2

[Michael Hendy (Hendikins)]

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

Comment 3

[Dwayne]

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

[Diego Biurrun]

NEWified as requested ;)

Comment 5

[Bob Clary [:bc] (inactive)]

Not evang -> default owner of Plugins

Comment 6

[Peter Lubczynski]

Attachment: simplified testcase

Comment 7

[Peter Lubczynski]

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?

Comment 8

[av (gone)]

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.

Comment 9

[av (gone)]

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.

Comment 10

[Peter Lubczynski]

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.

Comment 11

[av (gone)]

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.

Comment 12

[av (gone)]

Attachment: first try -- corrected logic in instantiating plugin

Comment 13

[av (gone)]

Rephrasing summary, setting milestone and keyword.

Comment 14

[Christopher Hoess (gone)]

This may also fix bug 65455.

Comment 15

[Peter Lubczynski]

r=peterl

Comment 16

[Chris Waterson]

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...

Comment 17

[av (gone)]

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

[Chris Waterson]

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.

Comment 19

[av (gone)]

Attachment: second try -- easier to read layout as per waterson's comment

Comment 20

[Chris Waterson]

thanks -- sr=waterson

Comment 21

[av (gone)]

Checked in to the trunk.

Comment 22

[shrirang khanzode]

verif on trunk, all plfs, 1113, url works now