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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(1 file, 3 obsolete files)

We want to be able to hold off OnStartRequest until we Resume.
Blocks: 384941
No longer blocks: 311566
Attached patch Like so (obsolete) — Splinter Review
Attachment #277346 - Flags: superreview?(cbiesinger)
Attachment #277346 - Flags: review?(cbiesinger)
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)

Attached patch Updated to comments (obsolete) — Splinter Review
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 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+
> 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.
Attached patch Updated to more comments (obsolete) — Splinter Review
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?
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.
Comment on attachment 277588 [details] [diff] [review]
Updated to more comments

Indeed.
Attachment #277588 - Flags: approval1.9? → approval1.9-
Attachment #277588 - Attachment is obsolete: true
Attachment #277616 - Flags: superreview?(cbiesinger)
Attachment #277616 - Flags: review?(cbiesinger)
+    // 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...
Er, that comment should have gone away.
Attachment #277616 - Flags: superreview?(cbiesinger)
Attachment #277616 - Flags: superreview+
Attachment #277616 - Flags: review?(cbiesinger)
Attachment #277616 - Flags: review+
isn't the loadgroup part of the comment still correct though?
Attachment #277616 - Flags: approval1.9?
Ah, yes.  I could just push that code down into DoReplaceWithProxy.  I assumed it wasn't there for a reason...
probably safer to just keep the loadgroup part of the comment in ProxyFailover
Attachment #277616 - Flags: approval1.9? → approval1.9+
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.

Attachment

General

Created:
Updated:
Size: