Closed Bug 341840 Opened 16 years ago Closed 16 years ago

microsummary service should cancel connections

Categories

(Firefox Graveyard :: Microsummaries, defect, P1)

2.0 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 2 beta2

People

(Reporter: myk, Assigned: dietrich)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 1 obsolete file)

The microsummary service should cancel connections after a specified timeout, and it should also cancel them when the user closes a window which is loading microsummaries, like the bookmark properties dialog.
Priority: -- → P1
Whiteboard: [swag: 2d]
Flags: blocking-firefox2?
This should be easier once the refactored code in bug 339543 gets checked in.
Depends on: 339543
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: Firefox 2 beta1 → Firefox 2 beta2
Whiteboard: [swag: 2d] → [swag: 2d] [myk-mss]
Component: Bookmarks → Microsummaries
Whiteboard: [swag: 2d] [myk-mss] → [swag: 2d]
Whiteboard: [swag: 2d] → [swag: 2d][at risk]
This is a patch for the first part of the bug: canceling requests after a timeout.
Attachment #231002 - Flags: review?(myk)
Assignee: myk → dietrich
Whiteboard: [swag: 2d][at risk] → [has patch][needs review myk]
Comment on attachment 231002 [details] [diff] [review]
cancel connections after a timeout

>+    // cancel loads that take too long
>+    // timeout specified in ms at browser.microsummies.requestTimeout, or 10 seconds

Nit: browser.microsummies should be browser.microsummary in this comment.

I wonder if it would make sense to use seconds rather than milliseconds in this pref, since that seems to be the maximum resolution users will ever need.  Mozilla developers might need millisecond-level resolution (f.e. I set the value to one millisecond at times during testing of this patch), but they can also edit the source file itself.


>+      timeout = pb.getBoolPref("browser.microsummary.requestTimeout");

getBoolPref -> getIntPref

(setting the pref has no effect if we retrieve the value with getBoolPref)


>+        LOG("timeout loading microsummary resource" + this._self.uri.spec + ", aborting request");

Nit: there should be a space after the word "resource" in this message.

Otherwise this looks great!  It's a good, minimal fix for the bug.
Attachment #231002 - Flags: review?(myk) → review-
Attachment #231002 - Attachment is obsolete: true
Attachment #231166 - Flags: review?(myk)
This is an untested patch for aborting the parsing of the iframe content when it's containing window is closed. I'm going to be gone for a week starting tomorrow, so won't be able to follow up on this until I get back.
Comment on attachment 231166 [details] [diff] [review]
review comments addressed

Looks good! r=myk
Attachment #231166 - Flags: review?(myk) → review+
Whiteboard: [has patch][needs review myk] → [has patch][checkin needed]
Comment on attachment 231166 [details] [diff] [review]
review comments addressed

Patch checked in to trunk.  Leaving bug open pending the second half of the fix.
Comment on attachment 231166 [details] [diff] [review]
review comments addressed

Notes for drivers considering approval request:

This patch for an Fx2 blocker makes the microsummary service abort the HTTP connections it uses to download microsummaries, generators, and HTML pages after a certain amount of time in order to prevent connections from staying open forever, tying up resources unnecessarily.  The default timeout is hardcoded to 15 seconds, but users can change it by setting the hidden (and not defined by default) pref browser.microsummary.requestTimeout.

This patch largely fixes this bug (the only remaining work is canceling hidden iframe parsing when the user closes a window containing a hidden iframe, which Dietrich has started to address with his followup WIP patch).

The patch presents a low risk of regression, since the behavior when aborting a connection is similar to the existing behavior when a connection fails on its own.  The one regression risk might be that 15 seconds turns out to be too short for a significant number of connections, creating too many false positive timeouts (i.e. timed out connections that would have succeeded if left alone a little longer).

So we should keep an eye out for bugs about "microsummaries not updating" or "microsummaries not always showing up in the bookmark dialog picker", which would be a manifestation of such a problem.

This patch was checked in Monday, July 31.
Attachment #231166 - Flags: approval1.8.1?
Comment on attachment 231166 [details] [diff] [review]
review comments addressed

a=drivers, please land on the MOZILLA_1_8_BRANCH.
Attachment #231166 - Flags: approval1.8.1? → approval1.8.1+
Fix checked in to branch.  Marking this bug fixed, as the primary issue (dangling connections, perhaps forever) has been addressed.  I filed bug 346804 and bug 346805 on remaining issues so that we can track them (and assess them to see if they are significant enough to block the release) separately.

Thanks for the fix Dietrich!
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [has patch][checkin needed]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.