177026 - [FIXr]MIME determination priorities are all confused

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

We should really content-sniff before extension-sniffing... that would solve so
many problems...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this

The only thing that bothers me here is a possible change in behavior... with
this patch, HTTP and FTP channels no longer extension-guess themselves.  For
document loads,the unknown decoder kicks in.  But if you're just a random Necko
consumer opening a channel, that doesn't work.	Which is not so cool, because
nsIChannel is frozen.

Thoughts?  Perhaps limit the change for those channels to the case when the
LOAD_DOCUMENT_URI flag is set?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I should note two points:

1)  Doing this maximizes the benefit of any future improvements to the unknown
    content decoder
2)  If we want, we could make the decoder smart enough to use the type from the
    MIME service _and_ the type it sniffs together to come up with a better
    guess (a la IE).

I really am worried about necko consumers, though... expecting everyone who
cares to instantiate a MIME service if we return "unknown" is not very cool...

I'm starting to like the DOCUMENT_URI idea more and more...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: alternate approach...

This still seems a little fishy... <sigh>... I wish this were not frozen....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: real alternate approach

Comment 5

[Boris Zbarsky [:bzbarsky]]

Of course another alternative is to change the channel impls to automatically
insert an nsUnknownDecoder converter between themselves and the ultimate
consumer if they get to calling OnStartRequest and the type is still unknown....
This may in fact be the way to go.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Ready for review

OK.  This one does not really change the interface contract assuming you're
calling GetContentType from OnStartRequest or later (and if you weren't before,
you were in trouble anyway).

Thoughts?  I like this one enough to ask for reviews.  ;)

I didn't change the file channel because that may have perf implications (loads
from jars, or something, eg).  Plus for local files extensions are more likely
to match reality....

Comment 7

[Darin Fisher]

yeah, i like this last patch.  it is not exactly like IE, since we'd only invoke
the unknown content decoder when the server fails to specify a content-type
header, but i think that is a good thing from the point-of-view of respecting
web standards.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Right.  That was sort of the goal of the exercise.  :)

I'm still thinking about how to make the unknown decoder play nice with
content-encoded data.. it's probably not a big issue for now (since no server I
know ever serves up data with a content-encoding but no content-type), but if we
ever want to support content-encoding on ftp:// urls we'll need that....

Do you like that patch enough to review, Darin?

Comment 9

[Darin Fisher]

Comment on attachment 104337 [details] [diff] [review]
Ready for review

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+    return GetTypeFromExtension(PromiseFlatCString(
>+             Substring(specStr, extLoc, specStr.Length() - extLoc - 1)
>+           ).get(), aContentType);
>   }
>   else
>+    return NS_ERROR_FAILURE;

nit: eliminate the "else"


>Index: netwerk/protocol/ftp/src/nsFTPChannel.cpp

>+                NS_ConvertASCIItoUCS2 from(UNKNOWN_CONTENT_TYPE);
>+                nsCOMPtr<nsIStreamListener> converter;
>+                rv = serv->AsyncConvertData(from.get(),

how about replacing |from| with a NS_LITERAL_STRING while you're at it.


>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

>+            NS_ConvertASCIItoUCS2 from(UNKNOWN_CONTENT_TYPE);
>+            nsCOMPtr<nsIStreamListener> converter;
>+            rv = serv->AsyncConvertData(from.get(),

ditto.


> NS_IMETHODIMP
> nsHttpChannel::GetContentType(nsACString &value)
> {
>     if (mResponseHead && !mResponseHead->ContentType().IsEmpty()) {
>         value = mResponseHead->ContentType();
>         return NS_OK;
>     }
> 
>     // the content-type can only be set to application/x-unknown-content-type 
>     // if there is data from which to infer a content-type.
>+    if (!mResponseHead) {
>+        value.Truncate();
>         return NS_ERROR_NOT_AVAILABLE;
>+    }
>+    
>     value = NS_LITERAL_CSTRING(UNKNOWN_CONTENT_TYPE);
>     return NS_OK;
> }

can you rework this function to avoid checking (mResponseHead != NULL) twice?

incidentally, are we now depending on the MIME service to handle the .dll
and .exe extension checks now?

>Index: netwerk/streamconv/converters/nsUnknownDecoder.cpp

>       nsCOMPtr<nsIChannel> aChannel = do_QueryInterface(request);
>-      if (!request) { NS_WARNING("QI failed"); return; }
>+      if (!aChannel) { NS_WARNING("QI failed"); return; }

good catch.  you might have actually prevented a future crash.

sr=darin w/ these things fixed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: better stronger faster

I left the NS_ConvertASCIItoUCS2 as-is because you can't NS_LITERAL_STRING a
macro.... 

> incidentally, are we now depending on the MIME service to handle the .dll

Yeah, we're now depending on the unknown decoder to hanle that.  It checks for
HTML content before looking at the extension, so we should be good (except the
times when a .dll or .exe returns plaintext content and expects us to detect it
as plaintext -- in that case, we'll hack the unknown decoder a bit).

Comment 11

[Darin Fisher]

Comment on attachment 104439 [details] [diff] [review]
better stronger faster

sr=darin

Comment 12

[rpotts (gone)]

+  // Now try to get an nsIURL so we don't have to do our own parsing
+  nsCOMPtr<nsIURL> url = do_QueryInterface(aURI);
+  if (url) {
+    nsCAutoString ext;
+    rv = url->GetFileExtension(ext);
+    if (NS_FAILED(rv)) return rv;
+    if (ext.IsEmpty()) {
+      *aContentType = nsnull;
+      return NS_ERROR_FAILURE;
+    }
+    return GetTypeFromExtension(ext.get(), aContentType);
+  }

nit:
*aContentType is set to NULL in this failure case... but there are earlier 
failures where it will be uninitialized...  perhaps, you should just null it out
at the top of the method to ensure it has a sane value.
also, shouldn't the return be NS_ERROR_NOT_AVAILABLE if the extension is empty?



-    // there won't be a response head if we've been cancelled
-    nsresult rv = mListener->OnStartRequest(this, mListenerContext);
-    if (NS_FAILED(rv)) return rv;
-
-    // install stream converter if required
-    ApplyContentConversions();
+    CallOnStartRequest();
     return NS_OK;
 }

shouldn't this be:
+   return CallOnStartRequest();

rather than always returning NS_OK...  Because, in the old code, a failure was
returned of OnStartRequest(...) failed...

otherwise this patch looks great!!
r=rpotts
-- rick

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix those too

Rick's right on all counts.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 104520 [details] [diff] [review]
Fix those too

marking r=rpotts, sr=darin

Comment 15

[Darin Fisher]

nice catch rick!  ignoring the return value of OnStartRequest would have broken
some things, since a failure returned from OnStartRequest is equivalent to an
explicit call to nsIRequest::cancel(...).

Comment 16

[Boris Zbarsky [:bzbarsky]]

checked in for 1.3a

Comment 17

[Chris Petersen]

Verified..