241592 - page with flash object and unsized image never finishes loading

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Dan M]

A simple page containing nothing but an embedded flash object and an image never
finishes loading if you don't have flash installed, and the image has no size
attributes. It's a somewhat contrived example, but it's curious that giving the
image a size fixes the problem. (This is why I've initially assigned this bug to
the layout component).

Comment 1

[Dan M]

Attachment: doesn't load

You'll have to uninstall Flash. (On my (WinXP) system, just hide
../program/plugins/NPSWF32.dll). Page never finishes loading.

Comment 2

[Dan M]

Attachment: does load

This one does finish loading. The only difference is this:
 <img src="http://www.creditunionsrock.com/logos/curocksbutton.gif"
-     align="absmiddle">
+     width="100" height="66" align="absmiddle">

Comment 3

[Dan M]

More notes: the |align| attribute has no effect. Also for best results, quit and
relaunch between tests. (Sometimes Mozilla will finish loading the "doesn't
load" example after having successfully loaded the "does load" example. But not
always.) I'm using a 20040423 Mozilla 1.8a trunk homebuild.

Comment 4

[zug_treno]

This looks like bug 241431. 

Comment 5

[Dan M]

Heh. Yeah, I missed that bug because I was searching for "flash" and
"image|gif|jpg" which was kind of dumb now that I think about it. Anyway these
two bugs do seem very similar. But my attempt to alter a local copy of
anandtech's site so that all its images had explicit sizes didn't help it finish
loading. I may have missed something; it's a big page and an example of image
overuse gone bad.

These two bugs are related. This one may be the same; it's at least a subset.
I'm marking this one dependent and adding a note to that other bug.

Comment 6

[Johnny Stenback (:jst)]

*** Bug 241431 has been marked as a duplicate of this bug. ***

Comment 7

[Johnny Stenback (:jst)]

Attachment: Make the default plugin kill the stream its given, and make killing a stream really kill it.

The reason for this was that since bug 216218 was fixed the browser kept
feeding the default plugin data forever since the default plugin never killed
the stream (it used to just die before due to unimplemented code that just
happened to kill a stream if it wasn't able to write data to a plugin). This
patch makes our default plugin kill the stream whenever its handed data on a
stream, and it also makes calling NPN_DestroyStream() always cancle the stream,
even if the stream is not suspended.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

-  if (mIsSuspended && NS_FAILED(status)) {
+  if (NS_FAILED(status)) {
     // We're suspended, and the stream was destroyed, or died for some
     // reason. Make sure we cancel the suspended request.

the comment seems to be outdated now

Comment 9

[Johnny Stenback (:jst)]

Good catch, I updated the comment to say:

    // The stream was destroyed, or died for some reason. Make sure we
    // cancel the underlying request.

Comment 10

[Darin Fisher]

Comment on attachment 147415 [details] [diff] [review]
Make the default plugin kill the stream its given, and make killing a stream really kill it.

> ns4xPluginStreamListener::OnStopBinding(nsIPluginStreamInfo* pluginInfo, 
...
>-  if (mIsSuspended && NS_FAILED(status)) {
>+  if (NS_FAILED(status)) {
>     // We're suspended, and the stream was destroyed, or died for some
>     // reason. Make sure we cancel the suspended request.

nit: the comment needs to be updated.  maybe just start with "The stream
was destroyed,..."


r=darin

Comment 11

[Darin Fisher]

doh!  i should have read the last two comments before posting :-)

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 147415 [details] [diff] [review]
Make the default plugin kill the stream its given, and make killing a stream really kill it.

sr+a=me for 1.7, this is ok for rc2.

/be

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

one more thing:
       NPP_PLUGIN_LOG(PLUGIN_LOG_NOISY,
                      ("NPP Write called: this=%p, npp=%p, pos=%d, len=%d, "
                       "buf=%s, return(written)=%d,  url=%s\n",
                       this, npp, streamPosition, numtowrite,
                       ptrStreamBuffer, writeCount, mNPStream.url));
 
+      if (!mStreamStarted) {
+        // The plugin called NPN_DestroyStream() from within
+        // NPP_WriteReady(), kill the stream.

Should the comment here say NPP_Write instead of NPP_WriteReady?

Comment 14

[Johnny Stenback (:jst)]

Fix checked in on the trunk.

Comment 15

[Johnny Stenback (:jst)]

Fixed on the branch now too.

Comment 16

[Johnny Stenback (:jst)]

(In reply to comment #13)
> one more thing:
>        NPP_PLUGIN_LOG(PLUGIN_LOG_NOISY,
>                       ("NPP Write called: this=%p, npp=%p, pos=%d, len=%d, "
>                        "buf=%s, return(written)=%d,  url=%s\n",
>                        this, npp, streamPosition, numtowrite,
>                        ptrStreamBuffer, writeCount, mNPStream.url));
>  
> +      if (!mStreamStarted) {
> +        // The plugin called NPN_DestroyStream() from within
> +        // NPP_WriteReady(), kill the stream.
> 
> Should the comment here say NPP_Write instead of NPP_WriteReady?

Yes, you're right. Fixed.