497563 - Crashes [@ BindStatementURI]

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase (uses enhanced privileges)

See testcase, which uses enhanced privileges, when clicking on any of the buttons, Mozilla crashes.

These crashes are currently ranked nr. 8 in the topcrashes list of Firefox3.5b99:
http://crash-stats.mozilla.com/query/query?do_query=1&product=Firefox&version=Firefox%3A3.5b99&date=&range_value=1&range_unit=weeks&query_search=signature&query_type=exact&query=

http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.5b99&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&do_query=1&signature=BindStatementURI%28mozIStorageStatement*%2C%20int%2C%20nsIURI*%29

Comments mention that the crashes happen on start-up, after trying to import bookmarks from Safari, subscribing to a live feed or opening the bookmarks manager.

http://crash-stats.mozilla.com/report/index/a6d68867-4771-4d90-b251-e261b2090610
0  	xul.dll  	BindStatementURI  	 toolkit/components/places/src/nsNavHistory.cpp:7225
1 	xul.dll 	nsNavHistory::GetUrlIdFor 	toolkit/components/places/src/nsNavHistory.cpp:1894
2 	xul.dll 	nsNavBookmarks::GetBookmarkedURIFor 	toolkit/components/places/src/nsNavBookmarks.cpp:2544
3 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
4 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2454 

http://crash-stats.mozilla.com/report/index/7356c957-eb10-49a2-b2b2-a4b122090609
0  	xul.dll  	BindStatementURI  	 toolkit/components/places/src/nsNavHistory.cpp:7225
1 	xul.dll 	nsAnnotationService::StartGetAnnotationFromURI 	toolkit/components/places/src/nsAnnotationService.cpp:1819
2 	xul.dll 	nsAnnotationService::GetPageAnnotation 	toolkit/components/places/src/nsAnnotationService.cpp:925
3 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
4 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2454 

http://crash-stats.mozilla.com/report/index/3d23770c-3541-43ff-bc84-1cc682090610
0  	xul.dll  	BindStatementURI  	 toolkit/components/places/src/nsNavHistory.cpp:7225
1 	xul.dll 	nsFaviconService::GetFaviconForPage 	toolkit/components/places/src/nsFaviconService.cpp:841
2 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
3 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2454 

It seems to me these methods should cause some javascript error, not cause the application to crash.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: fix

give js error instead crashing.

i have a followup patch coming that properly checks args in a bunch of callers of this, but this is enough for these scenarios. also, need to write a test for this.

Comment 2

[Shawn Wilsher :sdwilsh]

So, this method isn't public, so it is only our code that could be breaking us here.  Mind you, we should have assertions in this method to make sure things are not null, but we shouldn't slow down internal APIs by adding these null checks.  All our public API points should, in fact, be checking for null arguments and handling it accordingly.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

What's the blocking decision here? 200+ crashes in the past few days is a lot, but not a huge amount ... do we know what in our code is triggering the errors?

Comment 4

[Mike Shaver (:shaver emeritus)]

I wouldn't block on it, but would take the safe patch if it's ready in time -- even in an rc2 if we do one.

Comment 5

[Dietrich Ayala (:dietrich)]

Just mid-aired w/ Shaver, and agree.

I haven't fully investigated the causes. There are several paths to this crash shown in comment #0, and all sound like cases where a URI from an external source is missing or invalid: importing Safari bookmarks, loading feeds, etc.

It's ship time. We need to taking the crash-defense fix. We can file followups for the individual cases where we're not checking in the /browser code before passing to the back-end APIs.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: v2 + test

Comment 7

[Shawn Wilsher :sdwilsh]

Comment on attachment 382822 [details] [diff] [review]
v2 + test

>+++ b/toolkit/components/places/src/nsAnnotationService.cpp
> NS_IMETHODIMP
> nsAnnotationService::SetItemAnnotationString(PRInt64 aItemId,
>                                              const nsACString& aName,
>                                              const nsAString& aValue,
>                                              PRInt32 aFlags,
>                                              PRUint16 aExpiration)
> {
>+  NS_ENSURE_ARG(aItemId);
I don't think you actually want this one, right?

> NS_IMETHODIMP
> nsAnnotationService::SetItemAnnotationInt64(PRInt64 aItemId,
>                                             const nsACString& aName,
>                                             PRInt64 aValue,
>                                             PRInt32 aFlags,
>                                             PRUint16 aExpiration)
> {
>+  NS_ENSURE_ARG(aItemId);
ditto

> NS_IMETHODIMP
> nsAnnotationService::SetItemAnnotationDouble(PRInt64 aItemId,
>                                              const nsACString& aName,
>                                              double aValue,
>                                              PRInt32 aFlags,
>                                              PRUint16 aExpiration)
> {
>+  NS_ENSURE_ARG(aItemId);
ditto

> nsAnnotationService::SetItemAnnotationBinary(PRInt64 aItemId,
>                                              const nsACString& aName,
>                                              const PRUint8 *aData,
>                                              PRUint32 aDataLen,
>                                              const nsACString& aMimeType,
>                                              PRInt32 aFlags,
>                                              PRUint16 aExpiration)
> {
>+  NS_ENSURE_ARG(aItemId);
ditto

> nsAnnotationService::GetItemAnnotationString(PRInt64 aItemId,
>                                              const nsACString& aName,
>                                              nsAString& _retval)
> {
>+  NS_ENSURE_ARG(aItemId);
ditto

> nsAnnotationService::GetItemAnnotation(PRInt64 aItemId,
>                                        const nsACString& aName,
>                                        nsIVariant** _retval)
> {
>+  NS_ENSURE_ARG(aItemId);
ditto

> nsAnnotationService::GetItemAnnotationInt32(PRInt64 aItemId,
>                                             const nsACString& aName,
>                                             PRInt32 *_retval)
> {
>+  NS_ENSURE_ARG(aItemId);
ditto (I'll stop pointing this out).
Our internal functions should be able to handle an invalid id, and null->zero which would be invalid.  If they cannot handle an invalid id and crash, we should fix that too.

> nsAnnotationService::GetPageAnnotationType(nsIURI* aURI,
>                                            const nsACString& aName,
>                                            PRUint16* _retval)
> {
>+  NS_ENSURE_ARG(aURI);
need NS_ENSURE_ARG_PTR for _retval too

> nsAnnotationService::GetItemAnnotationType(PRInt64 aItemId,
>                                            const nsACString& aName,
>                                            PRUint16* _retval)
ditto

> nsAnnotationService::GetPageAnnotationDouble(nsIURI* aURI,
>                                              const nsACString& aName,
>                                              double *_retval)
ditto

> nsAnnotationService::GetItemAnnotationDouble(PRInt64 aItemId,
>                                              const nsACString& aName,
>                                              double *_retval)
ditto

> nsAnnotationService::GetPageAnnotationBinary(nsIURI* aURI,
>                                              const nsACString& aName,
>                                              PRUint8** aData,
>                                              PRUint32* aDataLen,
>                                              nsACString& aMimeType)
ditto for aData and aDataLen (and *sigh* for not following our normal style guidelines)

> nsAnnotationService::GetItemAnnotationBinary(PRInt64 aItemId,
>                                              const nsACString& aName,
>                                              PRUint8** aData,
>                                              PRUint32* aDataLen,
>                                              nsACString& aMimeType)
ditto

> nsAnnotationService::GetPageAnnotationInfo(nsIURI* aURI,
>                                            const nsACString& aName,
>                                            PRInt32 *aFlags,
>                                            PRUint16 *aExpiration,
>                                            nsACString& aMimeType,
>                                            PRUint16 *aStorageType)
ditto +aStorageType

> nsAnnotationService::GetItemAnnotationInfo(PRInt64 aItemId,
>                                            const nsACString& aName,
>                                            PRInt32 *aFlags,
>                                            PRUint16 *aExpiration,
>                                            nsACString& aMimeType,
>                                            PRUint16 *aStorageType)
ditto

>+++ b/toolkit/components/places/src/nsFaviconService.cpp
> nsFaviconService::GetFaviconData(nsIURI* aFaviconURI, nsACString& aMimeType,
>                                  PRUint32* aDataLen, PRUint8** aData)
> {
>+  NS_ENSURE_ARG(aFaviconURI);
>+  NS_ENSURE_ARG_POINTER(aData);
aDataLen is not checked

>+++ b/toolkit/components/places/src/nsLivemarkService.js
>   isLivemark: function LS_isLivemark(aFolderId) {
>+    if (!aFolderId)
>+      throw Cr.NS_ERROR_INVALID_ARG;
Same as before in the annotation service - we should be able to properly handle invalid id's, and this is only checking for an invalid id.

>   getFeedURI: function LS_getFeedURI(aFolderId) {
>+    if (!aFolderId)
>+      throw Cr.NS_ERROR_INVALID_ARG;
ditto

>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
>+nsNavBookmarks::InsertBookmark(PRInt64 aFolder,
>+                               nsIURI *aURI,
>+                               PRInt32 aIndex,
>                                const nsACString& aTitle,
>                                PRInt64 *aNewBookmarkId)
> {
>   // You can pass -1 to indicate append, but no other negative number is allowed
>   if (aIndex < nsINavBookmarksService::DEFAULT_INDEX)
>     return NS_ERROR_INVALID_ARG;
>   NS_ENSURE_ARG_POINTER(aNewBookmarkId);
>+  NS_ENSURE_ARG(aURI);
super nit: I'd prefer this as first, just because it'd follow the order of the arguments.

> nsNavBookmarks::GetFolderReadonly(PRInt64 aFolder, PRBool *aResult)
> {
>+  NS_ENSURE_ARG(aFolder);
should not check folder id

> nsNavBookmarks::SetFolderReadonly(PRInt64 aFolder, PRBool aReadOnly)
> {
>+  NS_ENSURE_ARG(aFolder);
ditto'

> nsNavBookmarks::InsertSeparator(PRInt64 aParent, PRInt32 aIndex,
>                                 PRInt64* aNewItemId)
> {
>+  NS_ENSURE_ARG(aParent);
>+  NS_ENSURE_ARG(aIndex);
shouldn't check parent or index

> nsNavBookmarks::GetIdForItemAt(PRInt64 aFolder, PRInt32 aIndex, PRInt64* aItemId)
> {
>+  NS_ENSURE_ARG(aFolder);
>+  NS_ENSURE_ARG(aIndex);
ditto

> nsNavBookmarks::RemoveChildAt(PRInt64 aParent, PRInt32 aIndex)
> {
>+  NS_ENSURE_ARG(aParent);
>+  NS_ENSURE_ARG(aIndex);
ditto

>-nsNavBookmarks::GetRemoveFolderTransaction(PRInt64 aFolder, nsITransaction** aResult)
>+nsNavBookmarks::GetRemoveFolderTransaction(PRInt64 aFolderId, nsITransaction** aResult)
> {
>+  NS_ENSURE_ARG(aFolderId);
no checking folder

> nsNavBookmarks::SetItemDateAdded(PRInt64 aItemId, PRTime aDateAdded)
> {
>+  NS_ENSURE_ARG(aItemId);
>+  NS_ENSURE_ARG(aDateAdded);
neither is a pointer, so not sure why we are checking here

> nsNavBookmarks::GetItemDateAdded(PRInt64 aItemId, PRTime *aDateAdded)
> {
>+  NS_ENSURE_ARG(aItemId);
just an id, do not check

> nsNavBookmarks::SetItemLastModified(PRInt64 aItemId, PRTime aLastModified)
> {
>+  NS_ENSURE_ARG(aItemId);
>+  NS_ENSURE_ARG(aLastModified);
no checking (I'll stop pointing this out)

> nsNavBookmarks::GetBookmarkedURIFor(nsIURI* aURI, nsIURI** _retval)
> {
>   *_retval = nsnull;
>+  NS_ENSURE_ARG(aURI);
hrm - we shouldn't be writing to _retval when we fail, so that line should be removed.

> nsNavBookmarks::GetBookmarkIdsForURITArray(nsIURI *aURI,
>                                          nsTArray<PRInt64> *aResult) 
> {
>+  NS_ENSURE_ARG(aURI);
ensure arg ptr on aResult

> nsNavBookmarks::GetBookmarkIdsForURI(nsIURI *aURI, PRUint32 *aCount,
>                                    PRInt64 **aBookmarks)
> {
>   *aCount = 0;
>   *aBookmarks = nsnull;
>+  NS_ENSURE_ARG(aURI);
shouldn't be setting aCount or aBookmarks (we could fail).  Also need to check that they are valid pointers.

> nsNavBookmarks::GetKeywordForURI(nsIURI* aURI, nsAString& aKeyword)
> {
>   aKeyword.Truncate(0);
we be writing on possible failure :(

> nsNavBookmarks::GetKeywordForBookmark(PRInt64 aBookmarkId, nsAString& aKeyword)
> {
>   aKeyword.Truncate(0);
ditto :(

Comment 8

[Martijn Wargers (dead)]

(In reply to comment #3)
> What's the blocking decision here? 200+ crashes in the past few days is a lot,
> but not a huge amount ... do we know what in our code is triggering the errors?

This is a worse crash than a crash you would normally get on a website or something.
This is a crash that apparently occurs every time the user wants to start up the browser. So basically they are unable to use Firefox anymore, because of this.

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: v3 + test

comments fixed

Comment 10

[Dietrich Ayala (:dietrich)]

re-requesting blocking due to comment #8.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 382853 [details] [diff] [review]
v3 + test

r=sdwilsh

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Martijn: do we know what *causes* those situations, though? We have 800,000 users on the beta, and only 200 crashes. That's higher than I'd like, but I'd like to know what those 200 users had in common.

a191 for landing this patch after someone can guarantee me that it won't break any tests!

Comment 13

[Martijn Wargers (dead)]

I don't think it's known what causes those situations, unfortunately.

Comment 14

[Marco Bonardo [:mak]]

FYI Safari migrator is doing a lazy and useless error check.
1091         rv |= NS_NewURI(getter_AddRefs(uri), url);
1092         PRInt64 id;
1093         rv |= aBookmarksService->InsertBookmark(aParentFolder, uri,

if NS_NewURI fails (and that can happen if safari has a bad uri) we pass a null uri to insertBookmarks... i hope there are not many places where we do this not existant error checking.

Comment 15

[Marco Bonardo [:mak]]

so, please while there fix Safari migrator.
Using rv |= makes sense when doing multiple calls to the same method, is crazy to do so when doing subsequent calls to different methods in differents components, and expecting each components takes bogus values from previous errors.

I think this is what caused the crashes, Safari users trying FX3.5, and migrating bookmarks. that could explain the low number.

Comment 16

[Marco Bonardo [:mak]]

@@ -447,21 +455,22 @@ LivemarkService.prototype = {
     catch (e) {
       return;
     }
     this._ans.setItemAnnotation(aFolderId, LMANNO_SITEURI, aSiteURI.spec,
                                 0, this._ans.EXPIRE_NEVER);
   },
 
   getFeedURI: function LS_getFeedURI(aFolderId) {
+    if (aFolderId < 1)
+      throw Cr.NS_ERROR_INVALID_ARG;
     if (this._ans.itemHasAnnotation(aFolderId, LMANNO_FEEDURI))
       return this._ios.newURI(this._ans.getItemAnnotation(aFolderId,
                                                           LMANNO_FEEDURI),
                               null, null);
-    return null;
   },

won't this cause a strinc JS warning about "not all code paths return a value"?

Comment 17

[timeless]

yes

Comment 18

[Dietrich Ayala (:dietrich)]

Attachment: v4 + test

fixes another wack test, adds safari migrator change, and fixes the livemark service warning.

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 382896 [details] [diff] [review]
v4 + test

>+++ b/browser/components/migration/src/nsSafariProfileMigrator.cpp
I'm going to pretend that I didn't see all those cases where we probably leak Carbon things...

r=sdwilsh

Comment 20

[timeless]

Comment on attachment 382896 [details] [diff] [review]
v4 + test

>+        rv = NS_NewURI(getter_AddRefs(uri), url);
>+        if (NS_SUCCEEDED(rv)) {
>+          PRInt64 id;
>+          rv |= aBookmarksService->InsertBookmark(aParentFolder, uri,
>+                                                  nsINavBookmarksService::DEFAULT_INDEX,
>+                                                  NS_ConvertUTF16toUTF8(title), &id);

no point in "|=" here given that it's only a success code (i.e. please use "="), all you're doing is giving a chance to corrupt the result value.

Comment 21

[Dietrich Ayala (:dietrich)]

pushed a tryserver build. made timeless' change.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

previous comment about pre-facto a191=beltzner assuming we don't blow up tryserver tests

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

(I still think this doesn't block, though we're going to see if we can squeeze it in to RC1)

Comment 24

[Dietrich Ayala (:dietrich)]

Attachment: v4.1 + test

the tryserver had 1 unit test failure. this was because the input checking of my patch caught a typo bug in the transaction service in the front-end.

here, should be s/_id/id/:

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/src/nsPlacesTransactionsService.js#851

the attached patch has the fix. carrying over review b/c it's a dead-simple typo error. will get post-facto additional review tomorrow.

Comment 25

[Dietrich Ayala (:dietrich)]

http://hg.mozilla.org/mozilla-central/rev/860a9acc39b1
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7272620d19cc

Comment 26

[Dietrich Ayala (:dietrich)]

note: the branch patch differed slightly due to trunk changes in the favicon service that are not on branch.

Comment 27

[Marco Bonardo [:mak]]

rv = NS_NewURI(getter_AddRefs(uri), url);
      13 +        if (NS_SUCCEEDED(rv)) {
      14 +          PRInt64 id;
      15 +          rv = aBookmarksService->InsertBookmark

actually this loses the previous errors, we are in a loop, the first rv = will overwrite any previous error in the loop, it should be

if (NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), url)) {
and the internal should still be rv |=
(even if ideally in case of bad errors we should simply NS_ENSURE_SUCCESS and in case of expectable errors we should continue;)

also previously where we do
aBookmarksService->CreateFolder
if it fails we should not try to put things inside the folder (again in this case we can't simply check rv since it could come from a previous loop.

Comment 28

[timeless]

realistically, i don't think looped things should affect state beyond the loop.

you're better served by:
+        if (NS_SUCCEEDED(NS_NewURI(getter_AddRefs(uri), url))) {
+          PRInt64 id;
+          aBookmarksService->InsertBookmark(aParentFolder,
+                                            uri,
+                                            nsINavBookmarksService::DEFAULT_INDEX,
+                                            NS_ConvertUTF16toUTF8(title),
+                                            &id);
+        }

The only useful version would be: if there is a single bookmark which is inserted then SUCCESS, else FAILURE.

Comment 29

[Dietrich Ayala (:dietrich)]

filed bug 497940 to fix the migrators.

this landed on both trees without problem -> FIXED.

Comment 30

[Martijn Wargers (dead)]

The patch contained a test, so this is in-testsuite+.

Comment 31

[Tony Chung [:tchung]]

martijn, can you verify the fix on 1.9.1 branch and trunk?  thanks.

Comment 32

[Martijn Wargers (dead)]

I'm getting these js errors now with the testcase:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getBookmarkedURIFor]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: https://bug497563.bugzilla.mozilla.org/attachment.cgi?id=382701 :: doe :: line 11"  data: no]
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.getPageAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: https://bug497563.bugzilla.mozilla.org/attachment.cgi?id=382701 :: doe2 :: line 18"  data: no]
Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIFaviconService.getFaviconForPage]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: https://bug497563.bugzilla.mozilla.org/attachment.cgi?id=382701 :: doe3 :: line 25"  data: no]
..which is good.

Comment 33

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv