221593 - win nsIconChannel.cpp should not call streamlistener methods synchronously

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

416                 aListener->OnDataAvailable(this, ctxt, inputStr, 0,
417                                            iconBuffer.Length());
418             aListener->OnStopRequest(this, ctxt, rv);

should be done via a proxy

Comment 1

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

Attachment: patch, unfinished

something like this, but I want to implement nsIconChannel::Open as well

Comment 2

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

Attachment: like this

Comment 3

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

(bug 222694 will bitrot this patch, so not requesting review yet)

Comment 4

[Mike Kaply [:mkaply]]

OK, 222694 is in. Should this be updated?

can the OS/2 version just go in? (bug 223128)

Comment 5

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

mkaply: yes, I'm updating this right now. os/2 can go in as-is.

Comment 6

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

Attachment: merge to trunk

Comment 7

[Darin Fisher]

Comment on attachment 134519 [details] [diff] [review]
merge to trunk

>Index: modules/libpr0n/decoders/icon/win/nsIconChannel.cpp

> #include "nsNetUtil.h"
...
>+#include "nsNetCID.h"
>+#include "nsIPipe.h"
>+#include "nsIInputStream.h"
>+#include "nsIOutputStream.h"

nsNetUtil.h already includes all of these.


>+  // Add ourself to the load group, if available
>+  if (mLoadGroup)
>+    mLoadGroup->AddRequest(this, nsnull);
>+
>+  rv = mPump->AsyncRead(this, ctxt);
>+  if (NS_FAILED(rv)) {
>+    // Break possibly reference cycle
>+    mListener = nsnull;
>+  }
>+  return rv;
>+}

you don't want to call AddRequest until you know AsyncOpen is going to 
return NS_OK.  see nsFileChannel::AsyncOpen for example.


>+nsresult nsIconChannel::MakeInputStream(nsIInputStream** _retval)
...
>+                // Now, create a pipe and stuff our data into it
>+                nsCOMPtr<nsIInputStream> inStream;
>+                nsCOMPtr<nsIOutputStream> outStream;
>+                rv = NS_NewPipe(getter_AddRefs(inStream), getter_AddRefs(outStream),
>+                                iconSize, iconSize);
>+                if (NS_SUCCEEDED(rv)) {
>+                  PRUint32 written;
>+                  rv = outStream->Write(buffer, iconSize, &written);
>+                  if (NS_SUCCEEDED(rv)) {
>+                    NS_ADDREF(*_retval = inStream);
>+                  }

you should make the input end of the pipe non-blocking, when it will
be used with AsyncOpen.  that way, the nsInputStreamPump will not need
to read from the pipe on a background thread to make it asynchronous ;-)

nsIInputStreamPump.idl explains this a bit.

i would add a nonBlockingInput parameter to MakeInputStream and pass
that to NS_NewPipe.


otherwise, this patch looks good to me.

Comment 8

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

Attachment: per darin's comments

Comment 9

[Darin Fisher]

Comment on attachment 135074 [details] [diff] [review]
per darin's comments

i think you got the sense of the nonBlocking argument backwards. 
nsIconChannel::Open must return a "blocking" input stream, so you want to pass
PR_FALSE in ::Open and PR_TRUE in ::AsyncOpen.

otherwise, the patch looks good.

Comment 10

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

"oops"
guess I thought of it as "blocking" rather than "nonblocking"

Comment 11

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

Attachment: use correct values for nonBlocking

Comment 12

[Mike Kaply [:mkaply]]

On OS/2, I had to use:

aContentType = NS_LITERAL_CSTRING("image/icon");

instead of

aContentType = NS_LITERAL_CSTRING("image/x-icon");

to get stuff to work.

Anyone have a clue why?

Comment 13

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

mkaply: yes - neil recently changed the windows iconchannel code to generate
.ico files - those have the image/x-icon content type, and are decoded by
nsICODecoder.cpp.

but os/2 was not changed and still creates some strange format for which the
type image/icon is used (and nsIconDecoder.cpp)

I don't think GetContentType was changed in this patch though...

Comment 14

[Mike Kaply [:mkaply]]

That code didn't change - it's more that when I ported the Windows code back to
OS/2, I noticed that difference and wondered why.

So if I understand correctly, image/x-icon is used, Mozilla assumes a Windows
icon and goes through the Windows icon format decoder.

Since the OS/2 format is not at all compatible with Windows, I wonder if I
should name the content type image/x-os2-icon or image/moz-os2-icon or something
like that?

Comment 15

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

>So if I understand correctly, image/x-icon is used, Mozilla assumes a Windows
>icon and goes through the Windows icon format decoder.

correct - also see bug 222694 which changed that (before, the windows icons were
converted to an intermediate format - that image/icon thing)

image/icon is not os/2 specific... the macos icon decoder also creates
image/icon icons.

Comment 16

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

Checking in win/nsIconChannel.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp,v  <-- 
nsIconChannel.cpp
new revision: 1.28; previous revision: 1.27
done
Checking in win/nsIconChannel.h;
/cvsroot/mozilla/modules/libpr0n/decoders/icon/win/nsIconChannel.h,v  <-- 
nsIconChannel.h
new revision: 1.6; previous revision: 1.5
done