169583 - URILoader should allow nsIContentHandler::HandleContent(...) to fail.

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[rpotts (gone)]

If nsIContentHandler::HandleContent(...) returns an error, the URILoader should
NOT abort the request.  Rather, it should continue looking for some other
consumer (ie. the unknown content handler)...

Currently, the URILoader unconditionally aborts the request once a
content-handler is located.  Regardless of whether the handler ACTUALLY accepts
the request.

Comment 1

[rpotts (gone)]

Attachment: check the return code from nsIContentHandler::HandleContent(...)

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 99770 [details] [diff] [review]
check the return code from nsIContentHandler::HandleContent(...)

r/sr=[e-mail]

/be

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

This patch will not work correctly without changes elsewhere, such as nsImapService.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Also, even some that on the surface implement it correctly
(nsInstallTrigger.cpp) will have problems, since there's no distinction made
between a ContentHandler() that didn't recognize the type, and one that hit an
error processing the content.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

The others (with the same problem as nsImapService) are nsNntpService and
nsMsgComposeContentHandler

Comment 6

[rpotts (gone)]

so Randall,

are you suggesting that HandleContent(...) have a specific failure code to
indicate the the handler rejected the content ?

such as NS_ERROR_WONT_HANDLE_CONTENT ?

and only look for other handlers if this failure is returned (rather than a
generic failure indicating a unrecoverable error)...

-- rick

Comment 7

[rpotts (gone)]

changing the 'component'...  This bug has *nothing* to do with the docshell...

it's an API ... so i guess embedding api's is better than nuthin...

Comment 8

[rpotts (gone)]

so after looking at the implementors of nsIContentHandler, it looks like the
'correct' code should look something like this: 

  rv = aContentHandler->HandleContent(...);
  if (rv != NS_ERROR_WONT_HANDLE_CONTENT) {
    *aAbortProcess = PR_TRUE;
    if (NS_FAILED(rv) {
      // The content handler has unexpectedly failed.  Cancel the
      // request -- just in case the handle did not!
      request->Cancel(rv);
    }
  }
  // If HandleContent returns WONT_HANDLE_CONTENT, do not abort.
  // Just return and keep looking for some other handler.


How does this look?

-- rick

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

rpotts: yeah, that's pretty much what I was driving at - use a specific error
code for "I don't grok this type" (and fix up the uses appropriately); then test
for that error.

Once that's done (including the implementers of HandleContent) I'll give an r=.

Comment 10

[rpotts (gone)]

I'm not sure why we would need to change the current implementors of
HandleContent(...) ?

None of them support this notion.  They all 'expect' to handle the data, so if
they fail we should just do what we currently do (and cancel the request)...

it's only for new/different content handlers than may want to conditionally
handle the content that NS_ERROR_WONT_HANDLE_CONTENT becomes important.

-- rick

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

The current handlers check the mimetype, and if they don't like it, most return
NS_OK, and one other returns NS_ERROR_FAILURE.  All of them can return other
errors if something in handling the content fails.

You need to change them to return NS_ERROR_WONT_HANDLE_CONTENT if they don't
like the mimetype, I believe.

Comment 12

[rpotts (gone)]

OK... for correctness i'll fix up the callers in the trunk patch.

but since it is currently an impossible situation with our implementations
(because we only call them for content-types that they have registered to
handle) i'm just going to fix up the URILoader for the branch patch -- to
minimize the number of changes...

Comment 13

[rpotts (gone)]

Attachment: branch patch...

This is a simplified version of the trunk patch -- to minimize code changes

Comment 14

[rpotts (gone)]

Attachment: trunk patch

Comment 15

[Darin Fisher]

Comment on attachment 100044 [details] [diff] [review]
trunk patch

r/sr=darin

Comment 16

[Darin Fisher]

Comment on attachment 100032 [details] [diff] [review]
branch patch...

r/sr=darin

Comment 17

[rpotts (gone)]

just checked the branch patch into the 1_0 branch

-- rick

Comment 18

[Jaime Rodriguez, Jr.]

bmartin: Suggetsed places we should test are --  unknown content handler dialogs
(downloads), and save-as and download mail attachments.

Comment 19

[Jaime Rodriguez, Jr.]

bmartin: Suggetsed places we should test are --  unknown content handler dialogs
(downloads), and save-as and download mail attachments.

Comment 20

[Jaime Rodriguez, Jr.]

Marking as fixed1.0.2 per Comment #17 From [e-mail].

Comment 21

[bmartin]

Updating Status to Resolved - Fixed based on comment #20 by Jaime

Comment 22

[bmartin]

Shirley sent out email last night at 11:11pm stating the following:

" I just verified bug 169858 on 2002-09-20-21-1.0 build. It's fixed and no
regressions have been found "

Marking this bug Verified Fixed based on Shirley's findings.

Comment 23

[bobj]

bug 169858 is unrelated to this bug.  Both of these bugs need to be verifed ASAP
for our embedding clients.

Comment 24

[Jaime Rodriguez, Jr.]

bmartin: bobj is correct. bug 169858 is unrelated to this bug. pls verify this
bug is fixed on the MOZILLA_1_0_20020920_TAG build. additonally,check unknown
content handler dialogs downloads), save-as and download mail attachments. Last,
once you have verified the fix, replace "fixed1.0.2" with the "verified1.0.2"
keyword. this let's us know that the fix has been verified on the branch. thanks!

Reopening, as the fix has not been verfied on the trunk or the branch, and based
on rick's comment "just checked the branch patch into the 1_0 branch", this has
only landed on the branch.

Comment 25

[bmartin]

I have not experienced any regressions with the normal Save As and unknown
content handler save dialogs.

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 100044 [details] [diff] [review]
trunk patch

r=jst

Comment 27

[rpotts (gone)]

i've just checked the trunk patch into the trunk...

closing out.

-- rick

Comment 28

[Gayatri Rimola]

Set QA Contact to Brenton Martin as per comment #24

Comment 29

[Michael Dunn]

Marking Verified1.0.2 per comments

Comment 30

[David Epstein]

Verified trunk patch against Mozilla 1.2b Mozilla/5.0
(Windows; U; WinNT4.0; en-US; rv:1.2b) Gecko/20021026 build. Looks good except
"wont handle content" code segment for
/cvsroot/mozilla/mailnews/compose/src/nsMsgComposeContentHandler.cpp needs to be
checked in. Rick will checkin when the tree is open. 

Comment 31

[Randell Jesup [:jesup] (needinfo me)]

This was on the branch for a long time, and the change that caused it to be
reopened isn't an issue for branch; removing mozilla1.0.2+

Comment 32

[XandreX]

This bug is blocking the "Make 1.0.2 not suck" bug :
<URL:http://bugzilla.mozilla.org/show_bug.cgi?id=161807> .

Is the september patch fully tested yet ?

Comment 33

[Boris Zbarsky [:bzbarsky]]

[e-mail], try reading the bug or something.  That patch has
been on the branch since Sept 20.

David, is comment #30 saying that the first hunk of the patch was just not
checked in to the trunk?

Comment 34

[rpotts (gone)]

Attachment: missing piece of the patch (oops)

Comment 35

[rpotts (gone)]

so, when i checked in the big patch... i missed this one file.

-- rick

Comment 36

[rpotts (gone)]

the patch to nsMsgCOmposeContentHandler.cpp as been checked in.

-- rick

Comment 37

[David Epstein]

verified. Mozilla 1.3b Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b)
Gecko/20030108