Closed Bug 369787 Opened 13 years ago Closed 13 years ago

calling nsHttpChannel::SetContentType on a closed channel doesn't work as expected.

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha5

People

(Reporter: sylvain.pasche, Assigned: sylvain.pasche)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

After the channel was dispatched and has no listener any more, calling nsHttpChannel::SetContentType() will set mContentCharsetHint, meaning the content type returned by GetContentType() is not changed.

mContentCharsetHint is used before opening the channel, but should not be used in case the channel is closed.
One problem is telling apart the "already closed" state and the "already closed, about to be reopened" state.  I don't really see anything in either the API or the code that prevents someone from reopening a channel after it's done...
we don't support reopening, even though this code might not enforce that people don't do that.
We should at least document that in the API then.  And maybe enforce it.  I'd be pretty happy with us adding a "was ever opened" flag or something at that point.

The reason I brought it up is that people are doing it, based on newsgroup posts I've seen.
Depends on: 372486
Attached patch v1 (obsolete) — Splinter Review
This needs bug 372486 to be checked in order to compile.
Assignee: nobody → sylvain.pasche
Status: NEW → ASSIGNED
Attachment #258960 - Flags: review?(cbiesinger)
Regarding your question about an empty http server handler, I guess it's ok doing nothing in it, as it will be dealt in a specific way:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/test/httpserver/httpd.js&rev=1.4&mark=1175-1192#1175
Comment on attachment 258960 [details] [diff] [review]
v1

please make BUGID and newType const

also, please use x-foo/x-bar

+function after_channel_closed() {
+  change_content_type();

put this in a try..catch (or try..finally) so that stop is called even if the test fails
Attachment #258960 - Flags: review?(cbiesinger) → review+
Attached patch v2Splinter Review
Attachment #258960 - Attachment is obsolete: true
Attachment #259482 - Flags: superreview?(darin.moz)
Comment on attachment 259482 [details] [diff] [review]
v2

sr=darin (sorry for not reviewing this earlier)
Attachment #259482 - Flags: superreview?(darin.moz) → superreview+
Whiteboard: [checkin needed]
mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp  1.308
mozilla/netwerk/test/unit/test_bug331825.js          1.5
mozilla/netwerk/test/unit/test_bug369787.js          1.1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha5
(In reply to comment #6)
> put this in a try..catch (or try..finally) so that stop is called even if the
> test fails

I should've pointed this out in onStopRequest as well :/ See bug 614717
You need to log in before you can comment on or make changes to this bug.