1366133 - Calling InternetGetConnectedStateExW() in ReadInternetOption() may cause hangs

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[(no longer active)]

In the BHR hang data I see hangs like this:

RtlpWaitOnAddress (in wntdll.pdb)<br>
RtlpWaitOnCriticalSection (in wntdll.pdb)<br>
RtlpEnterCriticalSectionContended (in wntdll.pdb)<br>
RtlEnterCriticalSection (in wntdll.pdb)<br>
IsDialUpConnection(CWxString *) (in wininet.pdb)<br>
InternetGetConnectedStateExW (in wininet.pdb)<br>
ReadInternetOption (in xul.pdb)<br>
nsWindowsSystemProxySettings::GetPACURI(nsACString &amp;) (in xul.pdb)<br>
mozilla::net::nsProtocolProxyService::ReloadPAC() (in xul.pdb)<br>
mozilla::net::nsProtocolProxyService::ReloadNetworkPAC() (in xul.pdb)<br>
mozilla::net::nsProtocolProxyService::Observe(nsISupports *,char const *,char16_t const *) (in xul.pdb)<br>
nsObserverList::NotifyObservers(nsISupports *,char const *,char16_t const *) (in xul.pdb)<br>
nsObserverService::NotifyObservers(nsISupports *,char const *,char16_t const *) (in xul.pdb)<br>
nsNotifyAddrListener::ChangeEvent::Run() (in xul.pdb)<br>
nsThread::ProcessNextEvent(bool,bool *) (in xul.pdb)<br>
NS_ProcessNextEvent(nsIThread *,bool) (in xul.pdb)<br>
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) (in xul.pdb)<br>
MessageLoop::RunHandler() (in xul.pdb)<br>
MessageLoop::Run() (in xul.pdb)<br>
nsBaseAppShell::Run() (in xul.pdb)<br>
nsAppShell::Run() (in xul.pdb)<br>
nsAppStartup::Run() (in xul.pdb)<br>
XREMain::XRE_mainRun() (in xul.pdb)<br>

As far as I can tell, we only call InternetGetConnectedStateExW() in <https://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp#75> so that we fill in the pszConnection member for the third argument to InternetQueryOptionW(), however per MSDN <https://msdn.microsoft.com/en-us/library/windows/desktop/aa385101(v=vs.85).aspx> that is an out argument...

Masatoshi, do you know if we actually need to initialize this field to the name of the modem connection?  I don't have a modem connection so I wouldn't be able to test what would happen if I changed this code myself...

Comment 1

[Masatoshi Kimura [:emk]]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #0)
> As far as I can tell, we only call InternetGetConnectedStateExW() in
> <https://searchfox.org/mozilla-central/rev/
> 24c443a440104dabd9447608bd41b8766e8fc2f5/toolkit/system/windowsproxy/
> nsWindowsSystemProxySettings.cpp#75> so that we fill in the pszConnection
> member for the third argument to InternetQueryOptionW(), however per MSDN
> <https://msdn.microsoft.com/en-us/library/windows/desktop/aa385101(v=vs.85).
> aspx> that is an out argument...

Actually this is an in-out parameter. MSDN is inaccurate as always.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa385146(v=vs.85).aspx
> pszConnection
>     Pointer to a string that contains the name of the RAS connection or NULL, which indicates the default or LAN connection, to set or *query* options on.
(emphasis mine)

> Masatoshi, do you know if we actually need to initialize this field to the
> name of the modem connection?

INTERNET_CONNECTION_MODEM will correspond to [Internet Options] > [Connections] > [Dial-up and Virtual Private Network settings] (so it requires a connection name to identify which connection setting we are operating on) and INTERNET_CONNECTION_LAN will correspond to [Local Area Network (LAN) settings].

> I don't have a modem connection so I wouldn't
> be able to test what would happen if I changed this code myself...

The "modem" doesn't have to be a physical modem. It might be a PPPoE connection, it might be a VPN connection. 

As an aside, we can remove the INTERNET_PER_CONN_FLAGS fallback because we no longer supports Windows Vista or earlier.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

I asked Honza about this bug and his response was:
> mayhemer: probably just make nsProtocolProxyService::Reload(Network)PAC happen on a background (best separate - new) thread?
> mayhemer: this is proxy related, maybe Gary should look at it

Gary, what do you think?

Comment 3

[Gary Chen [:xeonchen]]

(In reply to Valentin Gosu [:valentin] from comment #2)
> I asked Honza about this bug and his response was:
> > mayhemer: probably just make nsProtocolProxyService::Reload(Network)PAC happen on a background (best separate - new) thread?
> > mayhemer: this is proxy related, maybe Gary should look at it
> 
> Gary, what do you think?

I agree, I'll take this bug.

Two parts to do:

- Remove the INTERNET_PER_CONN_FLAGS fallback
- Move nsProtocolProxyService::Reload(Network)PAC to another thread

Comment 4

[(no longer active)]

Sounds great, thanks Masatoshi and Gary!

Comment 5

[Gary Chen [:xeonchen]]

Attachment: Bug 1366133 - Part 0: remove support for Windows Vista and earlier versions;

Review commit: https://reviewboard.mozilla.org/r/145210/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145210/

Comment 6

[Gary Chen [:xeonchen]]

Attachment: Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread;

Review commit: https://reviewboard.mozilla.org/r/145212/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/145212/

Comment 7

[Gary Chen [:xeonchen]]

Comment on attachment 8873793 [details]
Bug 1366133 - Part 0: remove support for Windows Vista and earlier versions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145210/diff/1-2/

Comment 8

[Gary Chen [:xeonchen]]

Comment on attachment 8873794 [details]
Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145212/diff/1-2/

Comment 9

[Gary Chen [:xeonchen]]

Comment on attachment 8873794 [details]
Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread;

After investigation, I found that |InternetGetConnectedStateExW()| is only called by two functions: |nsWindowsSystemProxySettings::GetPACURI| and |nsWindowsSystemProxySettings::GetProxyForURI|, and the latter is never called on main thread of Windows platform, therefore, only 2 occurrences of |GetPACURI| must be handled. I move the API calls and following steps into newly created function, |AsyncConfigureFromPAC|.

Comment 10

[Daniel Stenberg [:bagder]]

Comment on attachment 8873793 [details]
Bug 1366133 - Part 0: remove support for Windows Vista and earlier versions;

https://reviewboard.mozilla.org/r/145210/#review149590

::: toolkit/system/windowsproxy/nsWindowsSystemProxySettings.cpp:97
(Diff revision 2)
>      unsigned long size = sizeof(INTERNET_PER_CONN_OPTION_LISTW);
>      if (!InternetQueryOptionW(nullptr, INTERNET_OPTION_PER_CONNECTION_OPTION,
>                                &list, &size)) {
> -        if (GetLastError() != ERROR_INVALID_PARAMETER) {
> -            return NS_ERROR_FAILURE;
> +        return NS_ERROR_FAILURE;
> -        }
> +    }

Can you clarify exactly how this removes support for Vista and earlier (maybe in the commit message)? Are those versions more likely tyo return ERROR_INVALID_PARAMETER and require that second code block?

Comment 11

[Daniel Stenberg [:bagder]]

Comment on attachment 8873794 [details]
Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread;

https://reviewboard.mozilla.org/r/145212/#review149594

Comment 12

[Daniel Stenberg [:bagder]]

Just a heads-up: this bug is very similar to bug 1360164 so with a little care/luck I would say that we might be able to fix both at once.

Comment 13

[Gary Chen [:xeonchen]]

Comment on attachment 8873793 [details]
Bug 1366133 - Part 0: remove support for Windows Vista and earlier versions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145210/diff/2-3/

Comment 14

[Gary Chen [:xeonchen]]

Comment on attachment 8873794 [details]
Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145212/diff/2-3/

Comment 15

[Gary Chen [:xeonchen]]

Comment on attachment 8873794 [details]
Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145212/diff/3-4/

Comment 16

[Gary Chen [:xeonchen]]

Comment on attachment 8873793 [details]
Bug 1366133 - Part 0: remove support for Windows Vista and earlier versions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145210/diff/3-4/

Comment 17

[Gary Chen [:xeonchen]]

Comment on attachment 8873794 [details]
Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145212/diff/4-5/

Comment 18

[Gary Chen [:xeonchen]]

Comment on attachment 8873793 [details]
Bug 1366133 - Part 0: remove support for Windows Vista and earlier versions;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145210/diff/4-5/

Comment 19

[Gary Chen [:xeonchen]]

Comment on attachment 8873794 [details]
Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145212/diff/5-6/

Comment 20

[Gary Chen [:xeonchen]]

Comment on attachment 8873794 [details]
Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145212/diff/6-7/

Comment 21

[Gary Chen [:xeonchen]]

Comment on attachment 8873794 [details]
Bug 1366133 - Part 1: make nsISystemProxySettings::GetPACURI happen in another thread;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/145212/diff/7-8/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40c08de101f1
Part 0: remove support for Windows Vista and earlier versions; r=bagder
https://hg.mozilla.org/integration/autoland/rev/9a76c93fb1b5
Part 1: make nsISystemProxySettings::GetPACURI happen in another thread; r=bagder

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40c08de101f1
https://hg.mozilla.org/mozilla-central/rev/9a76c93fb1b5

Comment 25

[Jason Duell]

IIUC we're still going to delay getting system proxy info until the 1st necko channel needs it?  Would be it better to initialize the proxyProtocolSvc early in startup, so the 1st XHR, etc doesn't wind up waiting for init to finish?

Comment 26

[Daniel Stenberg [:bagder]]

Yes, I agree.

Even though this landed fix will already make it faster since there were other main-thread operations waiting to get done, so now they'll get done faster. This async but still init-on-first-use still risks delaying the first network request with 100ms.

That said, the first network request done is often also doing DNS and many other slow requests so those 100ms might not be very notable in a lot of cases.

I think we should file a new bug for this.

Comment 27

[Shian-Yow Wu [:swu]]

Filed follow up bug 1379872 per comment 26.

Comment 28

[q1]

FWIW, I just got an assertion crash in ~AsyncGetPACURIRequest () on line 369:

    367: ~AsyncGetPACURIRequest()
    368: {
    369:     MOZ_ASSERT(NS_IsMainThread() == mIsMainThreadOnly);
    370:     NS_ReleaseOnMainThreadSystemGroup(
    371:       "AsyncGetPACURIRequest::mServiceHolder", mServiceHolder.forget());
    372: }

on resume from hibernation on FF 56.0.1 x64 under Win7 SP1 x64. VS indicates that the code is running in the main thread in the chrome process. I have saved a dump about which I will answer questions (but not upload).

A comment on line 318 led me here:

    318: // Bug 1366133: make GetPACURI off-main-thread since it may hang on Windows platform

Comment 29

[Gary Chen [:xeonchen]]

(In reply to q1 from comment #28)
> FWIW, I just got an assertion crash in ~AsyncGetPACURIRequest () on line 369:
> 
>     367: ~AsyncGetPACURIRequest()
>     368: {
>     369:     MOZ_ASSERT(NS_IsMainThread() == mIsMainThreadOnly);
>     370:     NS_ReleaseOnMainThreadSystemGroup(
>     371:       "AsyncGetPACURIRequest::mServiceHolder",
> mServiceHolder.forget());
>     372: }
> 
> on resume from hibernation on FF 56.0.1 x64 under Win7 SP1 x64. VS indicates
> that the code is running in the main thread in the chrome process. I have
> saved a dump about which I will answer questions (but not upload).
> 
> A comment on line 318 led me here:
> 
>     318: // Bug 1366133: make GetPACURI off-main-thread since it may hang on
> Windows platform

thank you for reporting this, the issue has been fixed in bug 1405496.