Bug 1553537 Part 1 - Close our WinInet handles before terminating the file transfer thread. r=agashlin
authorMatt Howell <mhowell@mozilla.com>
Mon, 01 Jul 2019 18:30:25 +0000
changeset 540477 98db541275e5d242f5c5d3fff6ccacbfb5716481
parent 540476 8d6b6b345e249a49802444da28119c111621786d
child 540478 0b887601d2150c2827c0e24700aed45f2be89ded
push id11529
push userarchaeopteryx@coole-files.de
push dateThu, 04 Jul 2019 15:22:33 +0000
treeherdermozilla-beta@ebb510a784b8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersagashlin
bugs1553537
milestone69.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1553537 Part 1 - Close our WinInet handles before terminating the file transfer thread. r=agashlin Also reduce the timeout for terminating the thread to 5 seconds, because 10 seconds is too long to be completely hanging the UI. Differential Revision: https://phabricator.services.mozilla.com/D33844
other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp
--- a/other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp
+++ b/other-licenses/nsis/Contrib/InetBgDL/InetBgDL.cpp
@@ -19,16 +19,18 @@ typedef DWORD FILESIZE_T; // Limit to 4G
 #define FILESIZE_UNKNOWN (-1)
 
 #define MAX_STRLEN 1024
 
 HINSTANCE g_hInst;
 NSIS::stack_t*g_pLocations = NULL;
 HANDLE g_hThread = NULL;
 HANDLE g_hGETStartedEvent = NULL;
+HINTERNET g_hInetSes = NULL;
+HINTERNET g_hInetFile = NULL;
 volatile UINT g_FilesTotal = 0;
 volatile UINT g_FilesCompleted = 0;
 volatile UINT g_Status = STATUS_INITIAL;
 volatile FILESIZE_T g_cbCurrXF;
 volatile FILESIZE_T g_cbCurrTot = FILESIZE_UNKNOWN;
 CRITICAL_SECTION g_CritLock;
 UINT g_N_CCH;
 PTSTR g_N_Vars;
@@ -91,19 +93,30 @@ void Reset()
   TaskLock_AcquireExclusive();
 #ifndef ONELOCKTORULETHEMALL
   StatsLock_AcquireExclusive();
 #endif
   g_FilesTotal = 0; // This causes the Task thread to exit the transfer loop
   if (g_hThread)
   {
     TRACE(_T("InetBgDl: waiting on g_hThread\n"));
-    if (WAIT_OBJECT_0 != WaitForSingleObject(g_hThread, 10 * 1000))
+    if (WAIT_OBJECT_0 != WaitForSingleObject(g_hThread, 5 * 1000))
     {
       TRACE(_T("InetBgDl: terminating g_hThread\n"));
+      // Suspend the thread so that it's not still trying to use these handles
+      // that we're about to close out from under it.
+      SuspendThread(g_hThread);
+      if (g_hInetFile) {
+        InternetCloseHandle(g_hInetFile);
+        g_hInetFile = nullptr;
+      }
+      if (g_hInetSes) {
+        InternetCloseHandle(g_hInetSes);
+        g_hInetSes = nullptr;
+      }
       TerminateThread(g_hThread, ERROR_OPERATION_ABORTED);
     }
     CloseHandle(g_hThread);
     g_hThread = NULL;
   }
   g_FilesTotal = 0;
   g_FilesCompleted = 0;
   g_Status = STATUS_INITIAL;
@@ -265,17 +278,16 @@ void __stdcall InetStatusCallback(HINTER
       break;
   }
 #endif
 }
 
 DWORD CALLBACK TaskThreadProc(LPVOID ThreadParam)
 {
   NSIS::stack_t *pURL,*pFile;
-  HINTERNET hInetSes = NULL, hInetFile = NULL;
   DWORD cbio = sizeof(DWORD);
   DWORD previouslyWritten = 0, writtenThisSession = 0;
   HANDLE hLocalFile;
   bool completedFile = false;
 startnexttask:
   hLocalFile = INVALID_HANDLE_VALUE;
   pFile = NULL;
   TaskLock_AcquireExclusive();
@@ -323,63 +335,64 @@ startnexttask:
     if (0)
     {
 diegle:
       DWORD gle = GetLastError();
       //TODO? if (ERROR_INTERNET_EXTENDED_ERROR==gle) InternetGetLastResponseInfo(...)
       g_Status = STATUS_ERR_GETLASTERROR;
     }
 die:
-    if (hInetSes)
+    if (g_hInetSes)
     {
-      InternetCloseHandle(hInetSes);
+      InternetCloseHandle(g_hInetSes);
+      g_hInetSes = nullptr;
     }
     if (INVALID_HANDLE_VALUE != hLocalFile)
     {
       CloseHandle(hLocalFile);
     }
     StackFreeItem(pURL);
     StackFreeItem(pFile);
     return 0;
   }
 
-  if (!hInetSes)
+  if (!g_hInetSes)
   {
-    hInetSes = InternetOpen(USERAGENT, INTERNET_OPEN_TYPE_PRECONFIG, NULL, NULL, 0);
-    if (!hInetSes)
+    g_hInetSes = InternetOpen(USERAGENT, INTERNET_OPEN_TYPE_PRECONFIG, NULL, NULL, 0);
+    if (!g_hInetSes)
     {
       TRACE(_T("InetBgDl: InternetOpen failed with gle=%u\n"),
             GetLastError());
       goto diegle;
     }
-    InternetSetStatusCallback(hInetSes, (INTERNET_STATUS_CALLBACK)InetStatusCallback);
+    InternetSetStatusCallback(g_hInetSes, (INTERNET_STATUS_CALLBACK)InetStatusCallback);
 
     //msdn.microsoft.com/library/default.asp?url=/workshop/components/offline/offline.asp#Supporting Offline Browsing in Applications and Components
     ULONG longOpt;
     DWORD cbio = sizeof(ULONG);
-    if (InternetQueryOption(hInetSes, INTERNET_OPTION_CONNECTED_STATE, &longOpt, &cbio))
+    if (InternetQueryOption(g_hInetSes, INTERNET_OPTION_CONNECTED_STATE, &longOpt, &cbio))
     {
       if (INTERNET_STATE_DISCONNECTED_BY_USER&longOpt)
       {
         INTERNET_CONNECTED_INFO ci = {INTERNET_STATE_CONNECTED, 0};
-        InternetSetOption(hInetSes, INTERNET_OPTION_CONNECTED_STATE, &ci, sizeof(ci));
+        InternetSetOption(g_hInetSes, INTERNET_OPTION_CONNECTED_STATE, &ci, sizeof(ci));
       }
     }
 
     // Change the default connect timeout if specified.
     if(g_ConnectTimeout > 0)
     {
-      InternetSetOption(hInetSes, INTERNET_OPTION_CONNECT_TIMEOUT,
+      InternetSetOption(g_hInetSes, INTERNET_OPTION_CONNECT_TIMEOUT,
                         &g_ConnectTimeout, sizeof(g_ConnectTimeout));
     }
 
     // Change the default receive timeout if specified.
     if (g_ReceiveTimeout)
     {
-      InternetSetOption(hInetSes, INTERNET_OPTION_RECEIVE_TIMEOUT,
+      InternetSetOption(g_hInetSes, INTERNET_OPTION_RECEIVE_TIMEOUT,
                         &g_ReceiveTimeout, sizeof(DWORD));
     }
   }
 
   DWORD ec = ERROR_SUCCESS;
   hLocalFile = CreateFile(pFile->text, GENERIC_READ | GENERIC_WRITE,
                           FILE_SHARE_READ | FILE_SHARE_DELETE,
                           NULL, OPEN_ALWAYS, 0, NULL);
@@ -433,31 +446,31 @@ die:
     goto diegle;
   }
 
   // Tell the server to pick up wherever we left off.
   TCHAR headers[32] = _T("");
   _snwprintf(headers, 32, _T("Range: bytes=%d-\r\n"), previouslyWritten);
 
   TRACE(_T("InetBgDl: calling InternetOpenUrl with url=%s\n"), pURL->text);
-  hInetFile = InternetOpenUrl(hInetSes, pURL->text,
-                              headers, -1, IOUFlags |
-                              (uc.nScheme == INTERNET_SCHEME_HTTPS ?
-                               INTERNET_FLAG_SECURE : 0), 1);
-  if (!hInetFile)
+  g_hInetFile = InternetOpenUrl(g_hInetSes, pURL->text,
+                                headers, -1, IOUFlags |
+                                (uc.nScheme == INTERNET_SCHEME_HTTPS ?
+                                 INTERNET_FLAG_SECURE : 0), 1);
+  if (!g_hInetFile)
   {
     TRACE(_T("InetBgDl: InternetOpenUrl failed with gle=%u\n"),
           GetLastError());
     goto diegle;
   }
 
   // Get the file length via the Content-Length header
   FILESIZE_T cbThisFile;
   cbio = sizeof(cbThisFile);
-  if (!HttpQueryInfo(hInetFile,
+  if (!HttpQueryInfo(g_hInetFile,
                      HTTP_QUERY_CONTENT_LENGTH | HTTP_QUERY_FLAG_NUMBER,
                      &cbThisFile, &cbio, NULL))
   {
     cbThisFile = FILESIZE_UNKNOWN;
   }
   TRACE(_T("InetBgDl: file size=%d bytes\n"), cbThisFile);
 
   // Setup a buffer of size 256KiB to store the downloaded data.
@@ -465,38 +478,38 @@ die:
   // Use a 4MiB read buffer for the connection.
   // Bigger buffers will be faster.
   // cbReadBufXF should be a multiple of cbBufXF.
   const UINT cbReadBufXF = 4194304;
   BYTE bufXF[cbBufXF];
 
   // Up the default internal buffer size from 4096 to internalReadBufferSize.
   DWORD internalReadBufferSize = cbReadBufXF;
-  if (!InternetSetOption(hInetFile, INTERNET_OPTION_READ_BUFFER_SIZE,
+  if (!InternetSetOption(g_hInetFile, INTERNET_OPTION_READ_BUFFER_SIZE,
                          &internalReadBufferSize, sizeof(DWORD)))
   {
     TRACE(_T("InetBgDl: InternetSetOption failed to set read buffer size to %u bytes, gle=%u\n"),
           internalReadBufferSize, GetLastError());
 
     // Maybe it's too big, try half of the optimal value.  If that fails just
     // use the default.
     internalReadBufferSize /= 2;
-    if (!InternetSetOption(hInetFile, INTERNET_OPTION_READ_BUFFER_SIZE,
+    if (!InternetSetOption(g_hInetFile, INTERNET_OPTION_READ_BUFFER_SIZE,
                            &internalReadBufferSize, sizeof(DWORD)))
     {
       TRACE(_T("InetBgDl: InternetSetOption failed to set read buffer size ") \
             _T("to %u bytes (using default read buffer size), gle=%u\n"),
             internalReadBufferSize, GetLastError());
     }
   }
 
   for(;;)
   {
     DWORD cbio = 0, cbXF = 0;
-    BOOL retXF = InternetReadFile(hInetFile, bufXF, cbBufXF, &cbio);
+    BOOL retXF = InternetReadFile(g_hInetFile, g_bufXF, g_cbBufXF, &cbio);
     if (!retXF)
     {
       ec = GetLastError();
       TRACE(_T("InetBgDl: InternetReadFile failed, gle=%u\n"), ec);
       if (ERROR_INTERNET_CONNECTION_ABORTED == ec ||
           ERROR_INTERNET_CONNECTION_RESET == ec)
       {
         ec = ERROR_BROKEN_PIPE;
@@ -531,17 +544,17 @@ die:
       TRACE(_T("InetBgDl: 0 == g_FilesTotal, aborting transfer loop...\n"));
       ec = ERROR_CANCELLED;
       break;
     }
 
     cbXF = cbio;
     if (cbXF)
     {
-      retXF = WriteFile(hLocalFile, bufXF, cbXF, &cbio, NULL);
+      retXF = WriteFile(hLocalFile, g_bufXF, cbXF, &cbio, NULL);
       if (!retXF || cbXF != cbio)
       {
         ec = GetLastError();
         break;
       }
 
       StatsLock_AcquireExclusive();
       if (FILESIZE_UNKNOWN != cbThisFile) {
@@ -549,17 +562,18 @@ die:
       }
       writtenThisSession += cbXF;
       g_cbCurrXF += cbXF;
       StatsLock_ReleaseExclusive();
     }
   }
 
   TRACE(_T("InetBgDl: TaskThreadProc completed %s, ec=%u\n"), pURL->text, ec);
-  InternetCloseHandle(hInetFile);
+  InternetCloseHandle(g_hInetFile);
+  g_hInetFile = nullptr;
   if (ERROR_SUCCESS == ec)
   {
     if (INVALID_HANDLE_VALUE != hLocalFile)
     {
       CloseHandle(hLocalFile);
       hLocalFile = INVALID_HANDLE_VALUE;
     }
     StackFreeItem(pURL);