Closed
Bug 392837
Opened 17 years ago
Closed 17 years ago
[FIX]Make it possible to suspend HTTP channel right after AsyncOpen
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha8
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file, 3 obsolete files)
12.28 KB,
patch
|
Biesinger
:
review+
Biesinger
:
superreview+
bzbarsky
:
approval1.9+
|
Details | Diff | Splinter Review |
We want to be able to hold off OnStartRequest until we Resume.
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Comment 1•17 years ago
|
||
Attachment #277346 -
Flags: superreview?(cbiesinger)
Attachment #277346 -
Flags: review?(cbiesinger)
Comment 2•17 years ago
|
||
OK... I think there are a few problems with the patch - There's an AsyncAbort call in nsHttpChannel::AsyncOpen, which needs fixing too. That could be hard though. - There can be both a pump and a pending async call (when ReadFromCache is called from ProcessNotModified). I think it would be enough to always do mSuspendCount++ in Suspend to fix that (and in Resume to always check whether we need to do AsyncCall)
Assignee | ||
Comment 3•17 years ago
|
||
Attachment #277346 -
Attachment is obsolete: true
Attachment #277490 -
Flags: superreview?(cbiesinger)
Attachment #277490 -
Flags: review?(cbiesinger)
Attachment #277346 -
Flags: superreview?(cbiesinger)
Attachment #277346 -
Flags: review?(cbiesinger)
Comment 4•17 years ago
|
||
Comment on attachment 277490 [details] [diff] [review] Updated to comments would probably be a good idea to make sure that the channel is already opened by the time Suspend/Resume is called, so that calling it before asyncOpen still throws an error should Resume check that mSuspendCount > 0?
Attachment #277490 -
Flags: superreview?(cbiesinger)
Attachment #277490 -
Flags: superreview+
Attachment #277490 -
Flags: review?(cbiesinger)
Attachment #277490 -
Flags: review+
Assignee | ||
Comment 5•17 years ago
|
||
> would probably be a good idea to make sure that the channel is already opened Will do. > should Resume check that mSuspendCount > 0? Yeah, indeed. Will do.
Assignee | ||
Comment 6•17 years ago
|
||
Requesting 1.9 approval to make it possible to suspend/resume HTTP channels right after opening them, like we can do with other channels. Risk is pretty low.
Attachment #277490 -
Attachment is obsolete: true
Attachment #277588 -
Flags: approval1.9?
Comment 7•17 years ago
|
||
So... I'm not sure this does the right thing for the case where Connect() calls ResolveProxy() which calls AsyncResolve; it seems that suspending after that will be basically ignored.
Assignee | ||
Comment 8•17 years ago
|
||
Comment on attachment 277588 [details] [diff] [review] Updated to more comments Indeed.
Attachment #277588 -
Flags: approval1.9? → approval1.9-
Assignee | ||
Comment 9•17 years ago
|
||
Attachment #277588 -
Attachment is obsolete: true
Attachment #277616 -
Flags: superreview?(cbiesinger)
Attachment #277616 -
Flags: review?(cbiesinger)
Comment 10•17 years ago
|
||
+ // XXXbz so where does this codepath remove us from the loadgroup, + // exactly? Or change mStatus? Or whatever it should do? mStatus is handled by (Do)ReplaceWithProxy...
Assignee | ||
Comment 11•17 years ago
|
||
Er, that comment should have gone away.
Updated•17 years ago
|
Attachment #277616 -
Flags: superreview?(cbiesinger)
Attachment #277616 -
Flags: superreview+
Attachment #277616 -
Flags: review?(cbiesinger)
Attachment #277616 -
Flags: review+
Comment 12•17 years ago
|
||
isn't the loadgroup part of the comment still correct though?
Assignee | ||
Updated•17 years ago
|
Attachment #277616 -
Flags: approval1.9?
Assignee | ||
Comment 13•17 years ago
|
||
Ah, yes. I could just push that code down into DoReplaceWithProxy. I assumed it wasn't there for a reason...
Comment 14•17 years ago
|
||
probably safer to just keep the loadgroup part of the comment in ProxyFailover
Assignee | ||
Updated•17 years ago
|
Attachment #277616 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 15•17 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•