536256 - png image saved out to desktop is completely black

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

Might be the png lib, or it might be win32 widget, or it might just be the image itself. This bug is present in 3.5, 3.6, and trunk.

Visit here:

http://www.mozilla.org/projects/namoroka/

Drag the globe out to the windows desktop to save it. Image saved is completely black with no content.

This doesn't happen on all pngs, I've tried a few others and this is the only image I can reproduce it on.

Comment 1

[Glenn Randers-Pehrson]

According to pngcheck, the globe is a 32-bit RGBA (PNG color_type 6 bit_depth 8) with nothing unusual about it.

$ pngcheck -v deerpark-icon.png
File: deerpark-icon.png (16086 bytes)
  chunk IHDR at offset 0x0000c, length 13
    128 x 128 image, 32-bit RGB+alpha, non-interlaced
  chunk bKGD at offset 0x00025, length 6: red = 228 green = 236 blue = 236
  chunk pHYs at offset 0x00037, length 9: 2834x2834 pixels/meter (72 dpi)
  chunk tIME at offset 0x0004c, length 7:  1 Jun 2005 17:35:16 GMT
  chunk IDAT at offset 0x0005f, length 8192
    zlib:  deflated, 32K window, maximum compression
  chunk IDAT at offset 0x0206b, length 7767
  chunk IEND at offset 0x03ece, length 0
No errors detected in deerpark-icon.png (75.5% compression).
$

There are opaque, fully transparent, and some partially transparent pixels present.  The bKGD chunk describes an off-white background color.

Comment 2

[Glenn Randers-Pehrson]

The problem does not seem to be with the image itself.  I get reasonable behavior on Ubuntu with Firefox 3.5.6 (Gecko/20091215) which displays the image as an icon on the desktop, and with Windows XP pro with Minefield 3.7a1pre (private build Gecko/20091126) which displays the icon for whatever opens the image, and the image is OK when opened.

Comment 3

[Glenn Randers-Pehrson]

@jim, Just to be clear: you are seeing a black square, not a black circle,
right?  When I select the icon on the Ubuntu desktop, I see a very dark
circle, but if I click elsewhere on the desktop it brightens up.

Comment 4

[Jim Mathies [:jimm]]

(In reply to comment #3)
> @jim, Just to be clear: you are seeing a black square, not a black circle

Correct. The image is a black rect.

Comment 5

[Jim Mathies [:jimm]]

On the dragged out copy of this particular image, there's some truncation going on - 

File: G:\Users\jim\Desktop\png\deerpark-icon.png (255 bytes)
  chunk IHDR at offset 0x0000c, length 13
    128 x 128 image, 32-bit RGB+alpha, non-interlaced
  chunk bKGD at offset 0x00025, length 6
    red = 0x00e4, green = 0x00ec, blue = 0x00ec
  chunk pHYs at offset 0x00037, length 9: 2834x2834 pixels/meter (72 dpi)
  chunk tIME at offset 0x0004c, length 7:  1 Jun 2005 17:35:16 UTC
  chunk IDAT at offset 0x0005f, length 8192:  EOF while reading data
ERRORS DETECTED in G:\Users\jim\Desktop\png\deerpark-icon.png

Running the same test on a dragged out png from wikipedia's png page results in no error.

Comment 6

[Jim Mathies [:jimm]]

The problem here isn't with imagelib, the http channel in widget's drag object (nsDataObj) receives a "301 Moved Permanently" status for the image uri. The content length is 255 at that point which we return from our file stream Stat query. Then when we go to read the data off the network, we get the actual data but the length windows expects is still 255 bytes. I think what's needed here is better handling in nsDataObj for streams that redirect.

Comment 7

[Glenn Randers-Pehrson]

It's possible to ask libpng to execute a flush() after writing the last chunk, but from your comment #6 it seems that might not help.  Would you like to try anyway? I could make such a trial build.

Comment 8

[Glenn Randers-Pehrson]

If the lack of a flush() is the problem, then the regression would have occurred on 12 Sept 2008 with the checkin of bug #418900, when the flush() was removed.

Comment 9

[Glenn Randers-Pehrson]

Attachment: v01: request libpng to flush after writing IEND

Tryserver builds are at
https://build.mozilla.org/tryserver-builds/[e-mail]-bug-536256-v01-25Dec-flush/

Comment 10

[Phil Lacy]

Don't know if this is old news or not applicable 
but with XP in FF 3.5.6 I have a good image dragging to desktop I get a good image.
However rt clicking properties of web page's image says file size is 0 bytes

Comment 11

[(mostly gone) XtC4UaLL [:xtc4uall]]

(In reply to comment #8)
> If the lack of a flush() is the problem, then the regression would have
> occurred on 12 Sept 2008 with the checkin of bug #418900, when the flush() was
> removed.

i checked the builds around that date:
Gecko/20080911105329 Minefield/3.1b1pre => works
Gecko/20080912031847 Minefield/3.1b1pre => works
Gecko/20080913031911 Minefield/3.1b1pre => works
Gecko/20080914034157 Minefield/3.1b1pre => works

=> must have been some other checkin.

your tryserver build [Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20091227 Minefield/3.7a1pre ID:20091227170529] works fine though.

Comment 12

[Jim Mathies [:jimm]]

(In reply to comment #10)
> Don't know if this is old news or not applicable 
> but with XP in FF 3.5.6 I have a good image dragging to desktop I get a good
> image.
> However rt clicking properties of web page's image says file size is 0 bytes

I'm in 

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6 (.NET CLR 3.5.30729)

I see a good drag image, but I always get a 255 byte empty image on the desktop.

Live headers reports:

http://www.mozilla.org/projects/deerpark/deerpark-icon.png

GET /projects/deerpark/deerpark-icon.png HTTP/1.1
Host: www.mozilla.org
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6 (.NET CLR 3.5.30729)
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive


HTTP/1.x 301 Moved Permanently
Date: Tue, 29 Dec 2009 15:43:37 GMT
Expires: Tue, 29 Dec 2009 15:58:37 GMT
Cache-Control: max-age=900
Connection: Keep-Alive
Via: NS-CACHE-6.0:   4
Server: Apache
Location: http://www.mozilla.org/images/deerpark-icon.png
Content-Length: 255
Keep-Alive: timeout=20, max=972
Content-Type: text/html; charset=iso-8859-1


The data object has the old uri, and initiates a connection in the stream to retrieve it. That in turn returns a 301 http status error which we ignore. The contents of the image are probably the 301 error page returned with the status?

Comment 13

[Joe Drew (not getting mail)]

Comment on attachment 419299 [details] [diff] [review]
v01: request libpng to flush after writing IEND

If this works, let's just get it in.

Comment 14

[Glenn Randers-Pehrson]

This line is superfluous:

#define PNG_WRITE_FLUSH_SUPPORTED

since it's defined in pngconf.h
The doubly-defined macro might cause
problems on some platforms.

This one we need:

#define PNG_WRITE_FLUSH_AFTER_IEND_SUPPORTED

Comment 15

[Glenn Randers-Pehrson]

Attachment: v02: request libpng to flush after writing IEND

without the superflous macro definition

Comment 16

[Glenn Randers-Pehrson]

@joe, this was not incorporated in the libpng-1.4.0 upgrade so please transfer your +r to the v02 patch.

Comment 17

[Matt]

I found 2 regression ranges. In the first, firefox ceases to save to the desktop. A progress bar appears to try to save but doesn't. The icon never gets to the desktop. In the second, the save starts working again, and the black box icon appears.

First:

works:
2006-08-14-04-trunk

broken - never saved to the desktop:
2006-08-15-04-trunk

Second:

broken - never saved to the desktop:
2007-10-18-05-trunk

broken - black box icon:
2007-10-19-05-trunk

Comment 18

[Matt]

First 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=2006-08-14+03%3A00%3A00&maxdate=2006-08-15+05%3A00%3A00&cvsroot=%2Fcvsroot

Maybe Bug 203307.

Second 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=2007-10-18+04%3A00%3A00&maxdate=2007-10-19+06%3A00%3A00&cvsroot=%2Fcvsroot

Maybe Bug 396592.

Comment 19

[Jim Mathies [:jimm]]

(In reply to comment #18)
> First 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=2006-08-14+03%3A00%3A00&maxdate=2006-08-15+05%3A00%3A00&cvsroot=%2Fcvsroot
> 
> Maybe Bug 203307.
> 
> Second 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=2007-10-18+04%3A00%3A00&maxdate=2007-10-19+06%3A00%3A00&cvsroot=%2Fcvsroot
> 
> Maybe Bug 396592.

That might be it, I remember writing that patch. The Stat/GetContentLength call is needed for Vista and up, otherwise the drag doesn't work. I'd guess GetContentLength is returning a truncated length since the object the channel points to has moved.

Comment 20

[Joe Drew (not getting mail)]

Comment on attachment 420026 [details] [diff] [review]
v02: request libpng to flush after writing IEND

Is this still needed now that 1.4.1 has landed?

Comment 21

[Glenn Randers-Pehrson]

Attachment: v03: request libpng to flush after writing IEND

Yes, libpng-1.4.1 did not make any change in this area, so the patch is still needed.  It needed to be redone to apply cleanly because the offset is different.

Comment 22

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/28b6961febf1

Comment 23

[Dão Gottwald [:dao]]

Backed out. This made 90% of the tree orange. E.g. <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268294509.1268296331.25098.gz>, <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268296151.1268296831.26325.gz>.

Comment 24

[Joe Drew (not getting mail)]

It made the tree orange because it caused crashes. Someone'll probably have to load it up in a debugger to figure out what is going wrong.

It looks like it crashed in this mochitest:

141 INFO Running chrome://mochikit/content/chrome/content/xul/content/test/test_bug398289.html...

Comment 25

[Jim Mathies [:jimm]]

Attachment: async patch v.1

Comment 26

[Jim Mathies [:jimm]]

Attachment: async patch v.1

bz, this is widget code but I'd like someone from netwerk to look over my use of an async channel. AFAICT from stepping through the code and looking at logs, using nsIRequest::LOAD_FROM_CACHE insures I won't cause any network traffic with this. (Which is what I want.) Also, according to the idl it looks like I'm assured of getting an OnStopRequest here which (I'm counting on. If not I'll need to add some sort of short timeout to this.)

Comment 27

[Boris Zbarsky [:bzbarsky]]

> using nsIRequest::LOAD_FROM_CACHE insures I won't cause any network traffic
> with this

It doesn't, sorry.  This will hit the network if the data is not cached.

You could use nsICachingCannel::LOAD_ONLY_FROM_CACHE, but then you have to deal with failures if it's not in the cache, and for non-cacheable stuff (e.g ftp) you'd hit the network anyway.

But those hazards were present before too, no?  So it's not like this patch is making things worse in that regard.

Apart from that, OnDataAvailable needs to handle the case when the Read() call does not actually read aCount bytes: you need to call Read() in a loop until 0 bytes are read or an error is returned.

Also, NS_ProcessPendingEvents seems wrong here.  It's a busy-wait to call that in a loop, no?  You want NS_ProcessNextEvent.  And you need to check the return value; otherwise if shutdown happens while you're in this code you'll just never allow the program to exit.

>+    return (mChannelResult = NS_ERROR_FAILURE);

Probably better to write that as two statements, just so it's clear....

Should there be a Release() somewhere balancing the new AddRef in CreateStream?

Comment 28

[Jim Mathies [:jimm]]

(In reply to comment #27)
> > using nsIRequest::LOAD_FROM_CACHE insures I won't cause any network traffic
> > with this
> 
> It doesn't, sorry.  This will hit the network if the data is not cached.
> 
> You could use nsICachingCannel::LOAD_ONLY_FROM_CACHE, but then you have to
> deal with failures if it's not in the cache, and for non-cacheable stuff
> (e.g ftp) you'd hit the network anyway.
> 
> But those hazards were present before too, no?  So it's not like this patch
> is making things worse in that regard.

Yes, it should be fine. I just want to avoid hitting the network as much as possible. From testing drag-and-drop of images I didn't see any hits in the networking logs so it seems this is rare.

> 
> Apart from that, OnDataAvailable needs to handle the case when the Read()
> call does not actually read aCount bytes: you need to call Read() in a loop
> until 0 bytes are read or an error is returned.
> 
> Also, NS_ProcessPendingEvents seems wrong here.  It's a busy-wait to call
> that in a loop, no?  You want NS_ProcessNextEvent.  And you need to check
> the return value; otherwise if shutdown happens while you're in this code
> you'll just never allow the program to exit.

Ok, will do, thanks!

> 
> Should there be a Release() somewhere balancing the new AddRef in
> CreateStream?

Not in this case, we add ref it and hand the data to the drop target, which is responsible for releasing it through a STGMEDIUM specific release call Windows provides. The old code didn't need the AddRef because it started it's internal ref count at 1.

Comment 29

[Jim Mathies [:jimm]]

Attachment: async patch v.2

Comment 30

[Jim Mathies [:jimm]]

Attachment: async patch v.2

> Apart from that, OnDataAvailable needs to handle the case when the Read()
> call does not actually read aCount bytes: you need to call Read() in a loop
> until 0 bytes are read or an error is returned.

This issue has been fixed in the new patch. 

> Also, NS_ProcessPendingEvents seems wrong here.  It's a busy-wait to call
> that in a loop, no?  You want NS_ProcessNextEvent.  And you need to check
> the return value; otherwise if shutdown happens while you're in this code
> you'll just never allow the program to exit.

This was a bit more problematic. NS_ProcessNextEvent passes a wait flag which on Windows can get you waiting in the dispatch event loop for messages ofr an unlimited amount of time. Since NS_ProcessPendingEvents provides a timeout and calls thread->ProcessNextEvent in a loop w/mayWait set to false, I stuck with NS_ProcessPendingEvents.

The shutdown issue couldn't be resolved with a check of a return value since the NS_Proc. calls return a boolean indicating whether a message was processed. I looked through thread utils, app shell, and chromium's message loop for something I could check indicating shut down was needed, but didn't find anything. So I put together my own in base app shell.

I've tested this by setting the WaitForCompletion while loop to infinite, drag+dropped an image to the desktop, and then shut down the app. The main window remained responsive and closing the app succeeded (the while exited properly on ShouldQuit() == false).

Switching review? to roc as bz has already looked over the input stream code.

Comment 31

[Jim Mathies [:jimm]]

(In reply to comment #30)
> I've tested this by setting the WaitForCompletion while loop to infinite,
> drag+dropped an image to the desktop, and then shut down the app. The main
> window remained responsive and closing the app succeeded (the while exited
> properly on ShouldQuit() == false).

correction - "properly on ShouldQuit() == true)."

Comment 32

[Boris Zbarsky [:bzbarsky]]

> which on Windows can get you waiting in the dispatch event loop for messages
> ofr an unlimited amount of time.

Why is that a problem, compared to busy-waiting for an unlimited amount of time (which is what the patch seems to do)?

> since the NS_Proc. calls return a boolean indicating whether a message was
> processed.

Yes; the point is that if the thread is getting shut down you would get a return value indicating that no events were processed, and if you're using NS_ProcessNextEvent and told it to wait for an event, no event processed means there never will be one and you should just move on...  I'm not sure what the test in comment 30 actually tested; what needs testing is the scenario when we shut down before we get to the event we're waiting for, assuming that can happen at all.

Comment 33

[Boris Zbarsky [:bzbarsky]]

I don't think you need the "should quit" check in OnDataAvailable.

Comment 34

[Jim Mathies [:jimm]]

(In reply to comment #32)
> > since the NS_Proc. calls return a boolean indicating whether a message was
> > processed.
> 
> Yes; the point is that if the thread is getting shut down you would get a
> return value indicating that no events were processed, and if you're using
> NS_ProcessNextEvent and told it to wait for an event, no event processed
> means there never will be one and you should just move on...  I'm not sure
> what the test in comment 30 actually tested; what needs testing is the
> scenario when we shut down before we get to the event we're waiting for,
> assuming that can happen at all.

Hmm, that doesn't seem right. Maybe I'm missing something here, are you saying NS_ProcessNextEvent returning false accurately indicates that the nsIStreamListener has completed (OnStopRequest has been called), or that the app is shutting down? I would expect cases where there are no pending events w/out shut down and the stream listener still being active.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> are you saying NS_ProcessNextEvent returning false accurately indicates that
> the nsIStreamListener has completed (OnStopRequest has been called), or that
> the app is shutting down?

NS_ProcessNextEvent returning false when called with mayWait == true (the default) means the event queue that events are being processed on is shutting down, as far as I know.

The code in the attached patch has this form:

  while (!mChannelRead) {
    NS_ProcessPendingEvents(nsnull, PR_MillisecondsToInterval(100));
  }

and NS_ProcessPendingEvents does this:

  for (;;) {
    PRBool processedEvent;
    rv = thread->ProcessNextEvent(PR_FALSE, &processedEvent);
    if (NS_FAILED(rv) || !processedEvent)
      break;
    if (PR_IntervalNow() - start > timeout)
      break;
  }
  return rv;

which works great when used for its intended purpose: to process all pending events that are already posted and return (or return after the timeout if that happens before the events are all processed).  But you're calling it in a loop, so if there are no pending events you'll just busywait on the "PR_IntervalNow and function invocations, processedEvent is false, return to the outer loop waiting for mChannelRead" codepath.

Whereas if you called NS_ProcessNextEvent, it would either process an event if there is one, or wait until there is one and then process it, then return to your mChannelRead loop.  So if there are no events to process it would just wait for some to appear.

Or am I missing something?

Comment 36

[Jim Mathies [:jimm]]

Attachment: async patch v.3

notes on this:

1) My concern about getting hung up in WaitMessage() doesn't appear to be an issue, even if the window falls behind the window you are dragging to, so using NS_ProcessNextEvent with mayWait = true seems fine.

2) NS_ProcessNextEvent will have events to process after shutdown has been initiated, *but* shutdown will cancel the stream, so the OnStopRequest gets called. This breaks us out of the loop so there's no need for a shutdown check here and there's no need to check the return result NS_ProcessNextEvent.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think mChannelData should be an nsTArray<PRUint8>, that would be safer and simpler.

The nested event loop here is a little scary. What calls Stat()/Read()? Windows code that's running directly off the main event loop?

Comment 38

[Jim Mathies [:jimm]]

(In reply to comment #37)
> I think mChannelData should be an nsTArray<PRUint8>, that would be safer and
> simpler.

Will update that.

> 
> The nested event loop here is a little scary. What calls Stat()/Read()?
> Windows code that's running directly off the main event loop?

Yeah, it comes from COM via a windowing event we dispatch to a hidden COM window within our process.

Comment 39

[Jim Mathies [:jimm]]

(In reply to comment #37)
> I think mChannelData should be an nsTArray<PRUint8>, that would be safer and
> simpler.
> 
> The nested event loop here is a little scary. What calls Stat()/Read()?
> Windows code that's running directly off the main event loop?

Was that an r- or ..? I'm not sure what to do next here.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'd like to see a new patch with the nsTArray thing fixed.

Comment 41

[Jim Mathies [:jimm]]

Attachment: async patch v.4

Comment 42

[Jim Mathies [:jimm]]

Comment on attachment 551542 [details] [diff] [review]
async patch v.4

Updated to use nsTArray for the buffer.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think you need mDataSize. The length of the array can serve as mDataSize.

Comment 44

[Jim Mathies [:jimm]]

Attachment: async patch v.4

> I don't think you need mDataSize. The length of the array can serve as mDataSize.

Yep, that was unneeded. Updated patch.

Comment 45

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #44)
> Created attachment 551783 [details] [diff] [review] [diff] [details] [review]
> async patch v.4
> 
> > I don't think you need mDataSize. The length of the array can serve as mDataSize.
> 
> Yep, that was unneeded. Updated patch.

I need to update this comment in OnDataAvailable as well, I didn't notice it until I looked at the bugzilla diff:

// Accumulate the total size of the data buffer nsIStreamListener
// has promised thus far.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 551783 [details] [diff] [review]
async patch v.4

Review of attachment 551783 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: widget/src/windows/nsDataObj.cpp
@@ +151,4 @@
>  {
> +    // Accumulate the total size of the data buffer nsIStreamListener
> +    // has promised thus far.
> +    if (!mChannelData.SetLength(mChannelData.Length() + aCount))

I think you should assert that mChannelData.Length() == aOffset. You seem to rely on that below.

I'd change this code so that you do
  PRUint8* buffer = mChannelData.AppendElements(aCount);
and just increment 'buffer' instead of using mChannelData later. It'd be a tiny bit simpler.

@@ +245,5 @@
>  
> +  // Bytes left for Windows to read out of our buffer
> +  PRUint32 bytesLeft = mChannelData.Length() - mStreamRead;
> +  // Let Windows know what we will hand back, usually this is the entire buffer
> +  *nBytesRead = (bytesLeft <= nBytesToRead ? bytesLeft : nBytesToRead);

NS_MIN(bytesLeft, nBytesToRead)

Comment 47

[Jim Mathies [:jimm]]

landed on inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/6a43079a1c0f

bug 558565 & bug 622533 can be closed out with this one.

Comment 48

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/6a43079a1c0f