1337336 - pac file does not reload when network changes

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jun]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/602.3.12 (KHTML, like Gecko) Version/10.0.2 Safari/602.3.12

Steps to reproduce:

In Firefox connection settings, I choose `Use system proxy settings`, and on my Mac, I set `automatic proxy configuration ` and specified a pac file url. When I changed my Wi-Fi connection to another one which uses the same pac url but with different content, I can not load some sites.


Actual results:

Some site does not load when network changed.


Expected results:

When network changed, the PAC file content should be reloaded even if its URL is the same, which acts like Mac Safari.

Comment 1

[Loic]

zaczh, you reported this issue against FF54. Is it reproducible with release FF51?

Comment 2

[Jun]

Yes, I think so. I use FF for Mac two years ago and this bug exists ever since. Sorry maybe this bug report is a little too late. (In reply to Loic from comment #1)
> zaczh, you reported this issue against FF54. Is it reproducible with release
> FF51?

Comment 3

[Gary Chen [:xeonchen]]

Hi zaczh, would you attach the log?

Please follow [1] and set log modules to "timestamp,nsHttp:5,nsSocketTransport:5,nsStreamPump:5,nsHostResolver:5,proxy:5,nsNotifyAddr:5"

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/HTTP_logging#Using_aboutnetworking

Comment 4

[Jun]

Attachment: log.txt.0

Comment 5

[Jun]

Hi Gary, the log has been attached.
(In reply to Gary Chen [:xeonchen] from comment #3)
> Hi zaczh, would you attach the log?
> 
> Please follow [1] and set log modules to
> "timestamp,nsHttp:5,nsSocketTransport:5,nsStreamPump:5,nsHostResolver:5,
> proxy:5,nsNotifyAddr:5"
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Mozilla/Debugging/
> HTTP_logging#Using_aboutnetworking

Comment 6

[Jun]

Attachment: log.txt.1

Comment 7

[Jun]

See the last log file(log.txt.1). The previous log file was created with wrong module.

Comment 8

[Dragana Damjanovic [:dragana]]

Gary, do you want to take this bug?

Comment 9

[Gary Chen [:xeonchen]]

(In reply to Dragana Damjanovic [:dragana] from comment #8)
> Gary, do you want to take this bug?

Sure, I'll check this later.

Comment 10

[Gary Chen [:xeonchen]]

(In reply to Jun from comment #7)
> See the last log file(log.txt.1). The previous log file was created with
> wrong module.

Hi Jun,

I didn't see any "proxy" or "nsNotifyAddr" tag in the log, would you double confirm it?

Comment 11

[Jun]

Hi all,
I think I have found where is the issue. And here is my patch to solve this.


diff --git a/netwerk/base/nsProtocolProxyService.cpp b/netwerk/base/nsProtocolProxyService.cpp
--- a/netwerk/base/nsProtocolProxyService.cpp
+++ b/netwerk/base/nsProtocolProxyService.cpp
@@ -540,7 +540,12 @@ nsProtocolProxyService::Observe(nsISuppo
         nsCString converted = NS_ConvertUTF16toUTF8(aData);
         const char *state = converted.get();
         if (!strcmp(state, NS_NETWORK_LINK_DATA_CHANGED)) {
-            ReloadNetworkPAC();
+            // When network changes, we need to refresh the PAC url
+            // and load proxy settings from that url. We do this by
+            // reload these configures again.
+            nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
+            if (prefs)
+                PrefsChanged(prefs, nullptr);
         }
     }
     else {


When using `system proxy` settings, the original `ReloadNetworkPAC` call does nothing when I switched my Wi-Fi connection to another one. We need instead to refresh the proxy settings. I do this by simply reload proxy configures. I have tested this on my Mac and it works.

Comment 12

[Jun]

Attachment: proxy_issue.patch

Comment 13

[Gary Chen [:xeonchen]]

Comment on attachment 8846396 [details] [diff] [review]
proxy_issue.patch

Review of attachment 8846396 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsProtocolProxyService.cpp
@@ -540,4 @@
>          nsCString converted = NS_ConvertUTF16toUTF8(aData);
>          const char *state = converted.get();
>          if (!strcmp(state, NS_NETWORK_LINK_DATA_CHANGED)) {
> -            ReloadNetworkPAC();

According to what |ReloadNetworkPAC| does, it simply ignores the PAC script that uses "file://" or "data://". Are you using those scheme?

Comment 14

[Jun]

(In reply to Gary Chen [:xeonchen] (use ni? please) from comment #13)
> Comment on attachment 8846396 [details] [diff] [review]
> proxy_issue.patch
> 
> Review of attachment 8846396 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsProtocolProxyService.cpp
> @@ -540,4 @@
> >          nsCString converted = NS_ConvertUTF16toUTF8(aData);
> >          const char *state = converted.get();
> >          if (!strcmp(state, NS_NETWORK_LINK_DATA_CHANGED)) {
> > -            ReloadNetworkPAC();
> 
> According to what |ReloadNetworkPAC| does, it simply ignores the PAC script
> that uses "file://" or "data://". Are you using those scheme?

It's not the problem of `ReloadNetworkPAC`, it's `ReloadPAC` that does nothing. See its implementation below:


// nsIProtocolProxyService2
NS_IMETHODIMP
nsProtocolProxyService::ReloadPAC()
{
    nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
    if (!prefs)
        return NS_OK;

    int32_t type;
    nsresult rv = prefs->GetIntPref(PROXY_PREF("type"), &type);
    if (NS_FAILED(rv))
        return NS_OK;

    nsXPIDLCString pacSpec;
    if (type == PROXYCONFIG_PAC)
        prefs->GetCharPref(PROXY_PREF("autoconfig_url"), getter_Copies(pacSpec));
    else if (type == PROXYCONFIG_WPAD)
        pacSpec.AssignLiteral(WPAD_URL);

    if (!pacSpec.IsEmpty())
        ConfigureFromPAC(pacSpec, true);
    return NS_OK;
}

When using `System proxy` setting, Firefox will not using direct network link as other Apps like safari do, it still parses the system proxy pac settings, read the pac file and parse it using javascript. (See nsPacMan.cpp for details.) When network changes, this method does nothing, and the parsed proxy settings remains unchanged and thus cause problems.

Comment 15

[Daniel Stenberg [:bagder]]

Comment on attachment 8846396 [details] [diff] [review]
proxy_issue.patch

Review of attachment 8846396 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsProtocolProxyService.cpp
@@ +544,5 @@
> +            // and load proxy settings from that url. We do this by
> +            // reload these configures again.
> +            nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
> +            if (prefs)
> +                PrefsChanged(prefs, nullptr);

This not a good fix. If there's a problem in ReloadNetworkPAC(), it should be fixed there. Not the least because you're affecting all proxy types in one blow here and you're not doing it correctly.

You subsequently said that the bug is in ReloadPAC() for the system proxy case, so shouldn't the fix rather be in that function for that proxy setup case?

Comment 16

[Jun]

(In reply to Daniel Stenberg [:bagder] from comment #15)
> Comment on attachment 8846396 [details] [diff] [review]
> proxy_issue.patch
> 
> Review of attachment 8846396 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsProtocolProxyService.cpp
> @@ +544,5 @@
> > +            // and load proxy settings from that url. We do this by
> > +            // reload these configures again.
> > +            nsCOMPtr<nsIPrefBranch> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID);
> > +            if (prefs)
> > +                PrefsChanged(prefs, nullptr);
> 
> This not a good fix. If there's a problem in ReloadNetworkPAC(), it should
> be fixed there. Not the least because you're affecting all proxy types in
> one blow here and you're not doing it correctly.
> 
> You subsequently said that the bug is in ReloadPAC() for the system proxy
> case, so shouldn't the fix rather be in that function for that proxy setup
> case?

Hi Daniel,
  You are right. I have submitted a new patch.

diff --git a/netwerk/base/nsProtocolProxyService.cpp b/netwerk/base/nsProtocolProxyService.cpp
--- a/netwerk/base/nsProtocolProxyService.cpp
+++ b/netwerk/base/nsProtocolProxyService.cpp
@@ -1112,6 +1112,11 @@ nsProtocolProxyService::ReloadPAC()
         prefs->GetCharPref(PROXY_PREF("autoconfig_url"), getter_Copies(pacSpec));
     else if (type == PROXYCONFIG_WPAD)
         pacSpec.AssignLiteral(WPAD_URL);
+    else if (type == PROXYCONFIG_SYSTEM) {
+        if (mSystemProxySettings)
+            mSystemProxySettings->GetPACURI(pacSpec);
+        ResetPACThread();
+    }
 
     if (!pacSpec.IsEmpty())
         ConfigureFromPAC(pacSpec, true);

Comment 17

[Jun]

Attachment: Patch the ReloadPAC method to reload pac settings when network changed

Comment 18

[Daniel Stenberg [:bagder]]

Oh this looks so much better and very clean. Have you confirmed this (manually) to work for you? (on mac right?)

Comment 19

[Jun]

(In reply to Daniel Stenberg [:bagder] from comment #18)
> Oh this looks so much better and very clean. Have you confirmed this
> (manually) to work for you? (on mac right?)

Yes, I have tested and it works on my Mac.

But I have another question here: why do we need to parse the system pac file even the network configuration was set to `Using system proxy settings`? I noticed that parsing the pac file takes some time and during that time any webpage will fail to load. Shouldn't we just be ignorant of what proxy are configured of the system and load every request as if no proxy was set? I mean I am confused of the Firefox Network settings:
 1. `No proxy`
 2. `Auto-detect proxy` 
 3. `Using system proxy`
I think since we already have option 3, option 1 is no longer needed.

Comment 20

[Daniel Stenberg [:bagder]]

(In reply to Jun from comment #19)

> why do we need to parse the system pac
> file even the network configuration was set to `Using system proxy settings`?

If your system proxy settings says there's a PAC to use, shouldn't Firefox get it? It gets it again at network changes since the PAC may now be different than before.

> I noticed that parsing the pac file takes some time and during
> that time any webpage will fail to load. Shouldn't we just be ignorant of
> what proxy are configured of the system and load every request as if no
> proxy was set?

I don't think that's what "system proxy" means in this context. "System proxy" just means that Firefox figures out the proxy settings from the system (so that you don't have to manually specify host name/IP of the proxy or URL to the PAC or however you otherwise would configure your proxy), and then Firefox proceeds to use that setup when performing network operations.

If you don't need Firefox to use an explicit proxy, then you should select "no proxy".

> I mean I am confused of the Firefox Network settings:
>  1. `No proxy`
>  2. `Auto-detect proxy` 
>  3. `Using system proxy`
> I think since we already have option 3, option 1 is no longer needed.

No, since you can still ask Firefox to not use a proxy even if the system has proxy information.

Comment 21

[Jun]

(In reply to Daniel Stenberg [:bagder] from comment #20)
Hi Daniel,
   Thanks for your kindly reply. I indeed misunderstood the meaning of "System Proxy". But I still have a question here. By using other browsers(Safari, Chrome, Vivaldi), I find that their network settings is just a redirection to the system proxy setting. They don't have their own proxy settings. So my question is: If this is a technical difficulty that if an Application (on Mac) wants to obey the system proxy settings, it must not provide its own network proxy handling, and if it need to do so, then it should do all the network proxy job (including pac parsing, cache invalidating, etc.) by itself, just like Firefox? 
   I noticed that we still have some other bugs related to proxy, see https://bugzilla.mozilla.org/show_bug.cgi?id=1121441#c3

Comment 22

[Daniel Stenberg [:bagder]]

(In reply to Jun from comment #21)

> By using other
> browsers(Safari, Chrome, Vivaldi), I find that their network settings is
> just a redirection to the system proxy setting. They don't have their own
> proxy settings.

I just checked Chrome on my Linux machine and it certainly has "its own proxy settings" even when I select "use system proxy configuration" so I don't see any significant difference.

> So my question is: If this is a technical difficulty that if
> an Application (on Mac) wants to obey the system proxy settings, it must not
> provide its own network proxy handling, and if it need to do so, then it
> should do all the network proxy job (including pac parsing, cache
> invalidating, etc.) by itself, just like Firefox? 

There's a technical reason for this. If the browser has its own HTTP handling (like at least Firefox and Chrome do, I think Safari does too but in the Edge case it at least shares some of the HTTP stack with the OS), it must get the proxy information so that it can talk to the proxy directly. It cannot remain oblivious to that and it can't be handled transparently by the operating system somehow.

>    I noticed that we still have some other bugs related to proxy, see
> https://bugzilla.mozilla.org/show_bug.cgi?id=1121441#c3

Right, we have several proxy bugs outstanding. Fortunately they're not happening too frequently or aren't too annoying, but we absolutely have more work to do in this area. We try to set the "[proxy]" keyword in the whiteboard field for those bugs: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=[proxy]

Comment 23

[Daniel Stenberg [:bagder]]

Attachment: 0001-bug-1337336-fix-ReloadPAC-for-the-PROXYCONFIG_SYSTEM.patch

Here's Jun's unmodified patch in git format-patch format with a fixed commit message.

I also had it go through a try run with no issues: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f83366a8ed49a2d1b0b7ac9c04ce19dc982fba9

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/801d9e9ec6cd
Fix ReloadPAC() for the PROXYCONFIG_SYSTEM case. r=bagder

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/801d9e9ec6cd

Comment 26

[Gary Chen [:xeonchen]]

Thank you Jun, what a good job!

Comment 27

[Jun]

(In reply to Gary Chen [:xeonchen] (use ni? please) from comment #26)
> Thank you Jun, what a good job!

Thanks, it's my pleasure.