Benedict Hsieh [:benedict] Reporter
	Description • 14 years ago

We're getting EXCEPTION_IN_PAGE_ERROR from some accessing some mmap'd data. We need to handle these exceptions.

	Benjamin Smedberg
	Comment 1 • 14 years ago

__try/__catch, that is!

What happens on Linux in this case?

	(dormant account) Assignee
	Comment 2 • 14 years ago

(In reply to comment #1)
> __try/__catch, that is!
> 
> What happens on Linux in this case?

SIGBUS. I haven't seen a single such crash on Linux

	Brendan Eich [:brendan]
	Comment 3 • 14 years ago

(In reply to comment #0)
> We're getting EXCEPTION_IN_PAGE_ERROR from some accessing some mmap'd data. We
> need to handle these exceptions.

Why are we getting this exception?

/be

	(dormant account) Assignee
	Comment 4 • 14 years ago

(In reply to comment #3)
> (In reply to comment #0)
> > We're getting EXCEPTION_IN_PAGE_ERROR from some accessing some mmap'd data. We
> > need to handle these exceptions.
> 
> Why are we getting this exception?

ms docs are very vague about this. Supposedly it's due to files residing on network drives that disappear, but also due to virtual memory exhaustion(more likely).

http://msdn.microsoft.com/en-us/library/aa366801%28VS.85%29.aspx

	(dormant account) Assignee
	Comment 5 • 14 years ago

This should work. Unfortunately the only way to test this is to land it and see if the crashes go away. 

A similar fix needs to be applied to fasl code. Luckily due to whole-file checksum, it's more localized there.

	(dormant account) Assignee
	Comment 7 • 14 years ago

Requesting blocking status as this is a lowrisk crashfix.

	timeless
	Comment 8 • 14 years ago

Comment on attachment 477686 [details] [diff] [review]
Now with more try/catching

I don't think the ;'s are needed, you only use them 2/8 times.
>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(rv = NS_ERROR_FAILURE);

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE);

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)

>+MOZ_WIN_MEM_TRY_BEGIN
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)

>+#define MOZ_WIN_MEM_TRY_BEGIN __try {
>+#define MOZ_WIN_MEM_TRY_CATCH(cmd) }                                    \
>+  __except(GetExceptionCode()==EXCEPTION_IN_PAGE_ERROR ?            \
>+           EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH)   \
>+  {                                                                 \
>+    cmd;                                                            \
>+  }
>+#else
>+#define MOZ_WIN_MEM_TRY_BEGIN
>+#define MOZ_WIN_MEM_TRY_CATCH(cmd)
>+#endif

	timeless
	Comment 9 • 14 years ago

oh, and the second #define seems to have it's \ too far to the right.

perhaps:
+#define MOZ_WIN_MEM_TRY_BEGIN      __try {
+#define MOZ_WIN_MEM_TRY_CATCH(cmd) }                                \
+  __except(GetExceptionCode()==EXCEPTION_IN_PAGE_ERROR ?            \

	(dormant account) Assignee
	Comment 10 • 14 years ago

This patch adds try/catch to the code. This causes a mochitest failure:

ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_range_bounds.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - [object Event @ 0x74667c0 (native @ 0x6c3bef0)] at undefined:undefined

This failure doesn't occur if I change the defines to expand to { }(ie keep scoping, get rid of try/catch). The failure occurs even though no exception is caught(ie control flow remains the same). I am still trying to figure this out.

	Benjamin Smedberg
	Comment 11 • 14 years ago

That appears to be random-orange bug 608918, and probably isn't related to this bug at all. You see that failure on Windows only?

	(dormant account) Assignee
	Comment 12 • 14 years ago

(In reply to comment #11)
> That appears to be random-orange bug 608918, and probably isn't related to this
> bug at all. You see that failure on Windows only?

The try/catch isn't applicable on non-windows platforms.

	(dormant account) Assignee
	Comment 13 • 14 years ago

Minor whitespace cleanups. Added an NS_WARNING() to the catch. Made the try/catch defines expand to {} on non-windows to keep scoping the same across platforms.

	(dormant account) Assignee
	Comment 14 • 14 years ago

Looks like all of the fails I was worried about were known random-oranges. Going to land this.

	(dormant account) Assignee
	Comment 15 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/be3852cc225b

	Wayne Mery (:wsmwk)
	Comment 16 • 14 years ago

Taras. Is this solid enough to land in 1.9.2?  It should fix at least 2 thunderbird topcrashes, and probably more.  (eg bug 622523)

I haven't checked any of these https://crash-stats.mozilla.com/query/query?product=Thunderbird&version=ALL%3AALL&range_value=2&range_unit=weeks&date=01%2F04%2F2011+10%3A06%3A12&query_search=signature&query_type=contains&query=nsJAR&build_id=&process_type=any&hang_type=any&do_query=1

... except memcmp | nsZipArchive::GetItem(char const*) bp-8706682f-53d0-44e4-bfaf-e8d9a2110104

	(dormant account) Assignee
	Comment 17 • 14 years ago

(In reply to comment #16)
> Taras. Is this solid enough to land in 1.9.2?  It should fix at least 2
> thunderbird topcrashes, and probably more.  (eg bug 622523)
> 

yeah it seems solid on trunk.

> ... except memcmp | nsZipArchive::GetItem(char const*)
> bp-8706682f-53d0-44e4-bfaf-e8d9a2110104

why is this one excepted?

	Wayne Mery (:wsmwk)
	Comment 18 • 14 years ago

(In reply to comment #17)
> (In reply to comment #16)
> > Taras. Is this solid enough to land in 1.9.2?  It should fix at least 2
> > thunderbird topcrashes, and probably more.  (eg bug 622523)
> > 
> 
> yeah it seems solid on trunk.

great! can you make that happen please?


> > ... except memcmp | nsZipArchive::GetItem(char const*)
> > bp-8706682f-53d0-44e4-bfaf-e8d9a2110104
> 
> why is this one excepted?

(poor wording) I mean, I checked that specific crash, but none of the rest in 
https://crash-stats.mozilla.com/query/query?product=Thunderbird&version=ALL%3AALL&range_value=2&range_unit=weeks&date=01%2F04%2F2011+10%3A06%3A12&query_search=signature&query_type=contains&query=nsJAR&build_id=&process_type=any&hang_type=any&do_query=1

	Wayne Mery (:wsmwk)
	Comment 19 • 14 years ago

would like to see this land in 1.9.2 to bake and ship to resolve Thunderbird crashes.

	Daniel Veditz [:dveditz]
	Comment 20 • 14 years ago

I don't think we'd hold a release for this so please push for a landing. Marking "needed" so approval requests get expedited.

	Wayne Mery (:wsmwk)
	Comment 21 • 13 years ago

(Taras in comment #17)
> (In reply to comment #16) 
> > ... except memcmp | nsZipArchive::GetItem(char const*)
> > bp-8706682f-53d0-44e4-bfaf-e8d9a2110104
> 
> why is this one excepted?

Meaning that I *did* check it/contacted the reporter.
Taras, can you prepare a patch for branch?

	Michael Wu [:mwu]
	Comment 22 • 13 years ago

It's largely the same as the original patch, except a new try/catch is added for file extract since there is no zipcursor.

	(dormant account) Assignee
	Comment 23 • 13 years ago

Comment on attachment 537416 [details] [diff] [review]
Patch for 1.9.2

># HG changeset patch
># User Michael Wu <mwu@mozilla.com>
># Parent 36bf853240c7e65f9ec19d92fbaa05b6866dbab6
>Bug 598416 - add try/catch in places in nsZipReader.cpp that access mmaped memory directly r=bsmedberg try: -b do -p win32 -u all -t none
>
>diff --git a/modules/libjar/nsJARInputStream.cpp b/modules/libjar/nsJARInputStream.cpp
>--- a/modules/libjar/nsJARInputStream.cpp
>+++ b/modules/libjar/nsJARInputStream.cpp
>@@ -47,6 +47,9 @@
> #include "nsEscape.h"
> #include "nsIFile.h"
> #include "nsDebug.h"
>+#if defined(XP_WIN)
>+#include <windows.h>
>+#endif
> 
> /*---------------------------------------------
>  *  nsISupports implementation
>@@ -212,6 +215,7 @@ nsJARInputStream::Read(char* aBuffer, PR
>     *aBytesRead = 0;
> 
>     nsresult rv = NS_OK;
>+MOZ_WIN_MEM_TRY_BEGIN
>     switch (mMode) {
>       case MODE_NOTINITED:
>         return NS_OK;
>@@ -250,6 +254,7 @@ nsJARInputStream::Read(char* aBuffer, PR
>         }
>         break;
>     }
>+MOZ_WIN_MEM_TRY_CATCH(rv = NS_ERROR_FAILURE)
>     return rv;
> }
> 
>diff --git a/modules/libjar/nsZipArchive.cpp b/modules/libjar/nsZipArchive.cpp
>--- a/modules/libjar/nsZipArchive.cpp
>+++ b/modules/libjar/nsZipArchive.cpp
>@@ -59,6 +59,9 @@
> #include "stdlib.h"
> #include "nsWildCard.h"
> #include "nsZipArchive.h"
>+#if defined(XP_WIN)
>+#include <windows.h>
>+#endif
> 
> /**
>  * Global allocator used with zlib. Destroyed in module shutdown.
>@@ -314,7 +317,7 @@ nsZipItem*  nsZipArchive::GetItem(const 
>                 return 0;
>         }
>     }
>-
>+MOZ_WIN_MEM_TRY_BEGIN
>     nsZipItem* item = mFiles[ HashName(aEntryName, len) ];
>     while (item) {
>       if ((len == item->nameLength) && 
>@@ -322,8 +325,9 @@ nsZipItem*  nsZipArchive::GetItem(const 
>         return item; //-- found it
>       item = item->next;
>     }
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)
>   }
>-  return 0;
>+  return nsnull;
> }
> 
> //---------------------------------------------
>@@ -341,6 +345,7 @@ nsresult nsZipArchive::ExtractFile(nsZip
>   if (!mFd)
>     return NS_ERROR_FAILURE;
> 
>+MOZ_WIN_MEM_TRY_BEGIN
>   // Directory extraction is handled in nsJAR::Extract,
>   // so the item to be extracted should never be a directory
>   PR_ASSERT(!item->IsDirectory());
>@@ -373,8 +378,8 @@ nsresult nsZipArchive::ExtractFile(nsZip
>       rv = ResolveSymlink(outname);
> #endif
>   }
>-
>   return rv;
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)
> }
> 
> //---------------------------------------------
>@@ -445,7 +450,7 @@ nsresult nsZipFind::FindNext(const char 
> 
>   *aResult = 0;
>   *aNameLen = 0;
>-
>+MOZ_WIN_MEM_TRY_BEGIN
>   // we start from last match, look for next
>   while (mSlot < ZIP_TABSIZE)
>   {
>@@ -474,7 +479,7 @@ nsresult nsZipFind::FindNext(const char 
>       return NS_OK;
>     }
>   }
>-
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)
>   return NS_ERROR_FILE_TARGET_DOES_NOT_EXIST;
> }
> 
>@@ -526,7 +531,7 @@ nsresult nsZipArchive::BuildFileList()
>   PRUint8* buf;
>   PRUint8* startp = mFd->mFileData;
>   PRUint8* endp = startp + mFd->mLen;
>-
>+MOZ_WIN_MEM_TRY_BEGIN
>   PRUint32 centralOffset = 0;
>   for (buf = endp - ZIPEND_SIZE; buf > startp; buf--)
>   {
>@@ -582,6 +587,8 @@ nsresult nsZipArchive::BuildFileList()
> 
>   if (sig != ENDSIG)
>     return NS_ERROR_FILE_CORRUPTED;
>+
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)
>   return NS_OK;
> }
> 
>@@ -594,6 +601,7 @@ nsresult nsZipArchive::BuildSynthetics()
>     return NS_OK;
>   mBuiltSynthetics = true;
> 
>+MOZ_WIN_MEM_TRY_BEGIN
>   // Create synthetic entries for any missing directories.
>   // Do this when all ziptable has scanned to prevent double entries.
>   for (int i = 0; i < ZIP_TABSIZE; ++i)
>@@ -650,6 +658,7 @@ nsresult nsZipArchive::BuildSynthetics()
>       } /* end processing of dirs in item's name */
>     }
>   }
>+MOZ_WIN_MEM_TRY_CATCH(return NS_ERROR_FAILURE)
>   return NS_OK;
> }
> 
>@@ -666,6 +675,7 @@ nsZipHandle* nsZipArchive::GetFD()
> PRUint8* nsZipArchive::GetData(nsZipItem* aItem)
> {
>   PR_ASSERT (aItem);
>+MOZ_WIN_MEM_TRY_BEGIN
> 
>   //-- read local header to get variable length values and calculate
>   //-- the real data offset
>@@ -692,6 +702,7 @@ PRUint8* nsZipArchive::GetData(nsZipItem
>     return nsnull;
> 
>   return data + offset;
>+MOZ_WIN_MEM_TRY_CATCH(return nsnull)
> }
> 
> //---------------------------------------------
>diff --git a/modules/libjar/nsZipArchive.h b/modules/libjar/nsZipArchive.h
>--- a/modules/libjar/nsZipArchive.h
>+++ b/modules/libjar/nsZipArchive.h
>@@ -53,8 +53,21 @@
> #include "zipstruct.h"
> #include "nsAutoPtr.h"
> 
>+#if defined(XP_WIN)
>+#define MOZ_WIN_MEM_TRY_BEGIN __try {
>+#define MOZ_WIN_MEM_TRY_CATCH(cmd) }                                \
>+  __except(GetExceptionCode()==EXCEPTION_IN_PAGE_ERROR ?            \
>+           EXCEPTION_EXECUTE_HANDLER : EXCEPTION_CONTINUE_SEARCH)   \
>+  {                                                                 \
>+    NS_WARNING("EXCEPTION_IN_PAGE_ERROR in " __FUNCTION__);         \
>+    cmd;                                                            \
>+  }
>+#else
>+#define MOZ_WIN_MEM_TRY_BEGIN {
>+#define MOZ_WIN_MEM_TRY_CATCH(cmd) }
>+#endif
>+
> class nsZipFind;
>-
> struct PRFileDesc;
> 
> /**

	LegNeato
	Comment 24 • 13 years ago

Thanks a lot for getting to this guys!

	Michael Wu [:mwu]
	Comment 25 • 13 years ago

Optimistically marking fixed.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fd7fb656724f

	Al Billings [:abillings - ex-MoCo]
	Comment 26 • 13 years ago

What is the STR here, especially for 1.9.2?

	Karl Tomlinson (:karlt)
	Comment 27 • 13 years ago

(In reply to Taras Glek (:taras) from comment #2)
> (In reply to comment #1)
> > __try/__catch, that is!
> > 
> > What happens on Linux in this case?
> 
> SIGBUS. I haven't seen a single such crash on Linux

Bug 682607 perhaps.