Closed Bug 497563 Opened 11 years ago Closed 11 years ago

Crashes [@ BindStatementURI]

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: martijn.martijn, Assigned: dietrich)

References

Details

(4 keywords)

Crash Data

Attachments

(2 files, 4 obsolete files)

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.
Flags: blocking-firefox3.5?
Attached patch fix (obsolete) — Splinter Review
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.
Assignee: nobody → dietrich
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.
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?
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.
Flags: wanted-firefox3.5+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
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.
Flags: blocking-firefox3.5- → blocking-firefox3.5?
Flags: blocking-firefox3.5? → blocking-firefox3.5-
Attached patch v2 + test (obsolete) — Splinter Review
Attachment #382753 - Attachment is obsolete: true
Attachment #382822 - Flags: review?(sdwilsh)
Attachment #382822 - Flags: review?(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 :(
(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.
Attached patch v3 + test (obsolete) — Splinter Review
comments fixed
Attachment #382822 - Attachment is obsolete: true
Attachment #382853 - Flags: review?(sdwilsh)
re-requesting blocking due to comment #8.
Flags: blocking-firefox3.5- → blocking-firefox3.5?
Attachment #382853 - Flags: review?(sdwilsh) → review+
Comment on attachment 382853 [details] [diff] [review]
v3 + test

r=sdwilsh
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!
I don't think it's known what causes those situations, unfortunately.
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.
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.
@@ -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"?
yes
Attached patch v4 + test (obsolete) — Splinter Review
fixes another wack test, adds safari migrator change, and fixes the livemark service warning.
Attachment #382853 - Attachment is obsolete: true
Attachment #382896 - Flags: review?(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
Attachment #382896 - Flags: review?(sdwilsh) → review+
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.
pushed a tryserver build. made timeless' change.
previous comment about pre-facto a191=beltzner assuming we don't blow up tryserver tests
(I still think this doesn't block, though we're going to see if we can squeeze it in to RC1)
Flags: wanted1.9.1.x+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
Attached patch v4.1 + testSplinter Review
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.
Attachment #382896 - Attachment is obsolete: true
Attachment #382924 - Flags: review+
note: the branch patch differed slightly due to trunk changes in the favicon service that are not on branch.
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: x86 → All
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.
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.
filed bug 497940 to fix the migrators.

this landed on both trees without problem -> FIXED.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 498407
Depends on: 498722
Depends on: 498863
Flags: in-testsuite?
Target Milestone: --- → Firefox 3.6a1
The patch contained a test, so this is in-testsuite+.
Flags: in-testsuite? → in-testsuite+
martijn, can you verify the fix on 1.9.1 branch and trunk?  thanks.
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.
Status: RESOLVED → VERIFIED
Blocks: 333319
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Crash Signature: [@ BindStatementURI]
You need to log in before you can comment on or make changes to this bug.