pac file does not reload when network changes

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: zaczh, Assigned: zaczh)

Tracking

54 Branch
mozilla55
x86_64
macOS
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [proxy][necko-next])

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.

Updated

2 years ago
Component: Untriaged → Networking
Product: Firefox → Core

Updated

2 years ago
Whiteboard: [proxy]

Comment 1

2 years ago
zaczh, you reported this issue against FF54. Is it reproducible with release FF51?
Flags: needinfo?(zaczh)
(Assignee)

Comment 2

2 years ago
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?
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
See Also: → 1321841
(Assignee)

Comment 4

2 years ago
Posted file log.txt.0
(Assignee)

Comment 5

2 years ago
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
(Assignee)

Comment 6

2 years ago
Posted file log.txt.1
(Assignee)

Comment 7

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

Updated

2 years ago
See Also: → 1121441
(Assignee)

Updated

2 years ago
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Gary, do you want to take this bug?
Flags: needinfo?(xeonchen)
Whiteboard: [proxy] → [proxy][necko-next]
(In reply to Dragana Damjanovic [:dragana] from comment #8)
> Gary, do you want to take this bug?

Sure, I'll check this later.
Assignee: nobody → xeonchen
Flags: needinfo?(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?
(Assignee)

Comment 11

2 years ago
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.
(Assignee)

Comment 12

2 years ago
Posted patch proxy_issue.patch (obsolete) — Splinter Review

Updated

2 years ago
Attachment #8846396 - Flags: review?(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?
Attachment #8846396 - Flags: review?(xeonchen) → review?(daniel)
(Assignee)

Comment 14

2 years ago
(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 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?
Attachment #8846396 - Flags: review?(daniel) → review-
(Assignee)

Comment 16

2 years ago
(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);
Flags: needinfo?(zaczh)
(Assignee)

Comment 17

2 years ago
Attachment #8846396 - Attachment is obsolete: true
Oh this looks so much better and very clean. Have you confirmed this (manually) to work for you? (on mac right?)
(Assignee)

Comment 19

2 years ago
(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.
(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.
(Assignee)

Comment 21

2 years ago
(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
(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]
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
Attachment #8847076 - Attachment is obsolete: true
Attachment #8847528 - Flags: review+

Comment 24

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/801d9e9ec6cd
Fix ReloadPAC() for the PROXYCONFIG_SYSTEM case. r=bagder
Keywords: checkin-needed

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/801d9e9ec6cd
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thank you Jun, what a good job!
Assignee: xeonchen → zaczh
(Assignee)

Comment 27

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

Thanks, it's my pleasure. 
You need to log in before you can comment on or make changes to this bug.