Closed Bug 177026 Opened 17 years ago Closed 17 years ago

[FIXr]MIME determination priorities are all confused

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.3alpha

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 5 obsolete files)

We should really content-sniff before extension-sniffing... that would solve so
many problems...
Attached patch Something like this (obsolete) — Splinter Review
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?
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...
Blocks: 142681
Attached patch alternate approach... (obsolete) — Splinter Review
This still seems a little fishy... <sigh>... I wish this were not frozen....
Attached patch real alternate approach (obsolete) — Splinter Review
Attachment #104329 - Attachment is obsolete: true
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.
Attached patch Ready for review (obsolete) — Splinter 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....
Attachment #104327 - Attachment is obsolete: true
Attachment #104330 - Attachment is obsolete: true
Priority: -- → P1
Summary: MIME determination priorities are all confused → [FIX]MIME determination priorities are all confused
Target Milestone: --- → mozilla1.3alpha
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.
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 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.
Attachment #104337 - Flags: needs-work+
Attached patch better stronger faster (obsolete) — Splinter Review
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).
Attachment #104337 - Attachment is obsolete: true
Comment on attachment 104439 [details] [diff] [review]
better stronger faster

sr=darin
Attachment #104439 - Flags: superreview+
+  // 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
Attached patch Fix those tooSplinter Review
Rick's right on all counts.
Attachment #104439 - Attachment is obsolete: true
Comment on attachment 104520 [details] [diff] [review]
Fix those too

marking r=rpotts, sr=darin
Attachment #104520 - Flags: superreview+
Attachment #104520 - Flags: review+
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(...).
Blocks: 67940
Blocks: 61688
Summary: [FIX]MIME determination priorities are all confused → [FIXr]MIME determination priorities are all confused
checked in for 1.3a
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Component: Networking → File Handling
QA Contact: benc → petersen
Verified..
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.