341840 - microsummary service should cancel connections

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect, P1)

Description

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

This should be easier once the refactored code in bug 339543 gets checked in.

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: cancel connections after a timeout

This is a patch for the first part of the bug: canceling requests after a timeout.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: review comments addressed

Comment 5

[Dietrich Ayala (:dietrich)]

Attachment: wip patch for the close-window problem

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 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 231166 [details] [diff] [review]
review comments addressed

Looks good! r=myk

Comment 7

[Myk Melez [:myk] [@mykmelez]]

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 8

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 231166 [details] [diff] [review]
review comments addressed

a=drivers, please land on the MOZILLA_1_8_BRANCH.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

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!