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

VERIFIED FIXED in mozilla1.0.2

Status

()

Core
Document Navigation
P1
major
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: rpotts (gone), Assigned: rpotts (gone))

Tracking

({topembed+})

Trunk
mozilla1.0.2
x86
Windows 2000
topembed+
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] [ETA 09/21])

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

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

r/sr=brendan@mozilla.org

/be
Attachment #99770 - Flags: superreview+

Updated

15 years ago
Component: XP Miscellany → Embedding: Docshell

Updated

15 years ago
Keywords: mozilla1.0.2, topembed+

Updated

15 years ago
Blocks: 161807
This patch will not work correctly without changes elsewhere, such as nsImapService.
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.
The others (with the same problem as nsImapService) are nsNntpService and
nsMsgComposeContentHandler
(Assignee)

Comment 6

15 years ago
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
(Assignee)

Comment 7

15 years ago
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...
Component: Embedding: Docshell → Embedding: APIs
(Assignee)

Comment 8

15 years ago
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
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=.
Component: Embedding: APIs → Embedding: Docshell
(Assignee)

Comment 10

15 years ago
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
Component: Embedding: Docshell → Embedding: APIs
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.
Component: Embedding: APIs → Embedding: Docshell
(Assignee)

Comment 12

15 years ago
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...


(Assignee)

Comment 13

15 years ago
Created attachment 100032 [details] [diff] [review]
branch patch...

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

Updated

15 years ago
Attachment #99770 - Attachment is obsolete: true
(Assignee)

Comment 14

15 years ago
Created attachment 100044 [details] [diff] [review]
trunk patch

Comment 15

15 years ago
Comment on attachment 100044 [details] [diff] [review]
trunk patch

r/sr=darin
Attachment #100044 - Flags: superreview+

Comment 16

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

r/sr=darin
Attachment #100032 - Flags: superreview+

Updated

15 years ago
Attachment #100032 - Flags: review+
Attachment #100032 - Flags: approval+

Updated

15 years ago
Keywords: mozilla1.0.2 → mozilla1.0.2+
(Assignee)

Comment 17

15 years ago
just checked the branch patch into the 1_0 branch

-- rick

Comment 18

15 years ago
bmartin: Suggetsed places we should test are --  unknown content handler dialogs
(downloads), and save-as and download mail attachments.
Severity: normal → major
Keywords: edt1.0.2+, nsbeta1+
Priority: -- → P1
Whiteboard: [adt2] [ETA 09/21]
Target Milestone: --- → mozilla1.0.2

Comment 19

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

Comment 20

15 years ago
Marking as fixed1.0.2 per Comment #17 From rpotts@netscape.com.
Blocks: 154896
Keywords: fixed1.0.2

Comment 21

15 years ago
Updating Status to Resolved - Fixed based on comment #20 by Jaime
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 22

15 years ago
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.
Status: RESOLVED → VERIFIED

Comment 23

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

Comment 24

15 years ago
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 25

15 years ago
I have not experienced any regressions with the normal Save As and unknown
content handler save dialogs.
Comment on attachment 100044 [details] [diff] [review]
trunk patch

r=jst
Attachment #100044 - Flags: review+
(Assignee)

Comment 27

15 years ago
i've just checked the trunk patch into the trunk...

closing out.

-- rick
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Comment 28

15 years ago
Set QA Contact to Brenton Martin as per comment #24
QA Contact: brendan → bmartin

Comment 29

15 years ago
Marking Verified1.0.2 per comments
Keywords: fixed1.0.2 → verified1.0.2
QA Contact: bmartin → depstein

Comment 30

15 years ago
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. 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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+
Keywords: mozilla1.0.2+

Comment 32

15 years ago
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 ?
alexandre.ho_bugzilla@m4x.org, 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?
(Assignee)

Comment 34

15 years ago
Created attachment 108944 [details] [diff] [review]
missing piece of the patch (oops)
(Assignee)

Comment 35

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

-- rick
Attachment #108944 - Flags: superreview+
(Assignee)

Comment 36

15 years ago
the patch to nsMsgCOmposeContentHandler.cpp as been checked in.

-- rick
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Resolution: --- → FIXED

Comment 37

15 years ago
verified. Mozilla 1.3b Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b)
Gecko/20030108
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.