243277 - Offline: PAC: fails to load at startup when offline, then never tries again

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[benc]

STEPS:
Configure PAC.
Go offline.
Quit.
Restart (assumes that "When Starting up" is in default "remember previous
offline mode")

When you start again, PAC will error in javascript console, and PAC will never load.

I still need to do more research on the behavior of PAC in cache in this situation.

Comment 1

[Asa Dotzler [:asa]]

I'm unable to reproduce this problem in the latest aviary builds.

Comment 2

[Asa Dotzler [:asa]]

oops. Ignore that comment. wrong bug.

Comment 3

[Paul Dundas]

Combined with bug 240898 (Switching profile also switches Mozilla to work
offline), this is more severe. If a user arrives on a proxied network and
changes to a new profile (with a PAC), the "goes offline" bug prevents the PAC
from being loaded, and denies access to all proxied sites. Expert users may
guess that the proxy config URL needs to be reloaded, but others may not. 

I suspect that the proxy config should be reloaded (if absent? always?) when the
browser goes (back) online.

Comment 4

[benc]

That doesn't sound like it would be too hard to do...

Comment 5

[Darin Fisher]

Attachment: v1 patch

Very straightforward error handling.  If a load fails, then try again after some period of time.  Increase that interval exponentially up to a configurable maximum.  Try again when the PAC file is queried instead of running a nsITimer (that way we do not needlessly issue PAC loads when the browser is idle).

With this patch in place, it would be fairly trivial to add support for honoring HTTP cache expiration for the PAC file during a browser session.

Comment 6

[Darin Fisher]

BTW, the suggestion to reload PAC when the browser goes online is actually already implemented.  You can force a PAC reload today by toggling the work online/offline setting.  That works on the trunk at least.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 224166 [details] [diff] [review]
v1 patch

So once mLoadFailureCount is big enough (~32?) we'll try to do several PAC loads something like 5 secs apart, right?  I guess that's OK, since getting to something like 32 will take a few hours, if we're doing 5 minute timeouts between loads...

Comment 8

[Darin Fisher]

> So once mLoadFailureCount is big enough (~32?) we'll try to do several PAC
> loads something like 5 secs apart, right?

5, 10, 20, 40, 60... seconds apart, etc.  Notice the left shift.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 224166 [details] [diff] [review]
v1 patch

+  nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);

I wonder if a necko-internal helper method to get prefs would be useful, maybe also allowing to specify a default value...
(just a random thought, not necessarily for this bug)

bz:
>So once mLoadFailureCount is big enough (~32?) we'll try to do several PAC
>loads something like 5 secs apart, right? 

But the code sets interval to maxInterval if the shift returned 0, which happens after a lot of failures happened and the binary ones were shifted out of the number.


+    // Even if the PAC file could not be parsed, we did succeed in loading the
+    // data for it.
+    mLoadFailureCount = 0;

I think it would be simpler to move this to the beginning of the block. that way, reviewers don't have to make sure that there are no returns in between :->

Comment 10

[Darin Fisher]

fixed-on-trunk

Comment 11

[Darin Fisher]

Comment on attachment 224166 [details] [diff] [review]
v1 patch

I think it would be good to get this into the 1.8 branch for FF2.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 224166 [details] [diff] [review]
v1 patch

I'd let it bake for another week or two; if no regressions then, let's put it on branch.

Comment 13

[Darin Fisher]

fixed1.8.1

Comment 14

[Darin Fisher]

patch reverted on 1.8 branch to try to fix orange.  the problem might be due to another check-in... not sure.

Comment 15

[Darin Fisher]

this patch was not the source of the orange.  patch landed on 1.8 branch again.