431346 - File -> Import -> Browsing History crash Firefox [@ nsCOMPtr.h:931]

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect, P1)

Description

[Carsten Book [:Tomcat]]

Attachment: crash stack

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008042814 Firefox/3.0pre

Steps to reproduce:
-> File -> Import -> Safari 
-> Select "Browsing History>
--> Crash

Comment 1

[Marcia Knous [:marcia]]

This might be 10.5 only, as I do not crash importing data from Safari using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008042904 Minefield/3.0pre.

Comment 2

[Dietrich Ayala (:dietrich)]

see the URL, this started on the day bug X was checked in. from the reports, seems like this can happen even when singly deleting URLs from the history sidebar.

Comment 3

[Dietrich Ayala (:dietrich)]

(In reply to comment #2)
> see the URL, this started on the day bug X was checked in. 

er, bug 251337

Comment 4

[Ed Lee :Mardak]

This is the offending line of the patch from bug 251337:

http://hg.mozilla.org/mozilla-central/index.cgi/rev/5e8fa7e3f219#l3.123

 if (mHistoryTransaction) {
   // Delete the transaction and cause it to commit
*  delete mHistoryTransaction;
   mHistoryTransaction = nsnull;
 }

where mHistoryTransaction is created earlier with..
mHistoryTransaction = new mozStorageTransaction(mDBConn, PR_TRUE);

From the stack trace..
0   libtoolkitcomps.dylib         	0x0a639479 nsCOMPtr<mozIStorageConnection>::get_DerivedSafe() const + 9 (nsCOMPtr.h:931)
1   libtoolkitcomps.dylib         	0x0a63948f nsCOMPtr<mozIStorageConnection>::operator nsDerivedSafe<mozIStorageConnection>*() const + 17 (nsCOMPtr.h:862)
2   libtoolkitcomps.dylib         	0x0a6394fd mozStorageTransaction::~mozStorageTransaction() + 17 (mozStorageHelper.h:81)

It looks like it's checking mConnection here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/storage/public/mozStorageHelper.h&rev=1.2&mark=81#79
if (mConnection && mHasTransaction && ! mCompleted) {

that was added by bug 384224 to check for non-null connection (which should be mDBConn of nsDownlaodManager.)


Any idea if this is happening when things are shutting down?

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #4)

> 
> Any idea if this is happening when things are shutting down?
> 

it's not. i could reproduce using the steps in comment #0.

Comment 6

[Shawn Wilsher :sdwilsh]

The problem is this:
2   libtoolkitcomps.dylib         	0x0a6394fd mozStorageTransaction::~mozStorageTransaction() + 17 (mozStorageHelper.h:81)
3   libtoolkitcomps.dylib         	0x0a639583 mozStorageTransaction::~mozStorageTransaction() + 17 (mozStorageHelper.h:87)

Somehow, it's hitting that twice?  Are we calling this on two different threads perhaps?  Otherwise, I don't see how we could call that twice given the code.

Comment 7

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

Does this happen when migrating over from Safari (ie: on profile creation)? If not, then I don't *think* it blocks, it just sucks. If so, renom.

Also, if we have to back out bug 251337 to fix this, I'd be fine with doing that as much as it would pain me.

Comment 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I believe that we should back out the aging patch and observe the effects on crash stats, to confirm or refute dietrich's hypothesis that it is also responsible for the single-delete crash cases.  We can debug this with a locally-patched build without much difficulty, and that would put our default state to be "ship without aging, and with fewer poorly-understood crashes".  I assert that we should (vastly) prefer that state to the current one, given our desire to ship.

Comment 9

[Ed Lee :Mardak]

Alternatively, we can just back out the mozStorageTransaction part. According to the comments, I believe this crash will happen when you select multiple entries in the Library and hit delete.

Comment 10

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Does that put us in a state that has had any testing?  That we are in "impossible" call sequences here makes me want to revert very conservatively.  If the parallel debugging gives us confidence that aging-minus-transaction-changes (are the transaction changes not necessary for aging? if not, why were they taken?) is a good risk-reward tradeoff for inclusion, we can take it then, but I'm willing to get shouty about the need to retreat to known-better states rather than working to preserve late-breaking feature additions.  Post-beta additions need to be very safe, because we will lack widespread testing before we're ready to ship, and I don't think that the aging patch passes that test at this point -- for all its many virtues, which are many indeed.

If we knew that the patch had these side-effects, we would not have taken it, I'm pretty sure.  I don't think that we should always go backwards, but if we can't construct a solid explanation of why the bug happens, I think going forward with more speculative changes is the wrong approach at this late date.  Wish it weren't so. :/

Comment 11

[Shawn Wilsher :sdwilsh]

This crash looks a lot like something Mano was talking about when I was working on bug 405497 and why locks were (wrongly) added in Bug 382073.  Maybe he can provide more details...

It's my opinion that this patch exposed a bug in places that is not understood (given the fact that the aging code for the DM is straight-forward).  This is just a gut instinct though, and I haven't found anything to back that up.

Comment 12

[Carsten Book [:Tomcat]]

JS Stack from this crash:

(gdb) call DumpJSStack()
0 [native frame]
1 anonymous(aOuter = [object Object], 3)
["chrome://browser/content/migration/migration.js":367]
    this = [object ChromeWindow @ 0x11a8da80 (native @ 0x11a8cc5c)]

Comment 13

[Carsten Book [:Tomcat]]

Attachment: stack when deleting entries from history menu

ok, this crash is not only on import, the problem is also when you select one or more entries from the history menu and when you delete this entries (like the comments in the crash reports).

My steps to reproduce:
- Selected 10 (or so) entries from the History Libary
- Context Menu -> Delete 
--> Crash

Comment 14

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

Blocking per comment 13

Comment 15

[Ed Lee :Mardak]

What does this mean?

2   libtoolkitcomps.dylib         	0x0a70c4fd mozStorageTransaction::~mozStorageTransaction() + 17 (mozStorageHelper.h:81)
3   libtoolkitcomps.dylib         	0x0a70c583 mozStorageTransaction::~mozStorageTransaction() + 17 (mozStorageHelper.h:87)

frame 3 is at the close } of the ~mozStorageTransaction destructor which calls the destructor for frame 2 and now mDBConn is junk. I'm assuming that's because the first destructor (frame 3) killed mDBConn.... ??

Comment 16

[Ed Lee :Mardak]

(Well, why is the destructor calling the destructor in the first place.)

Shawn: Should we just back out the mozStorageTransaction part that you suggested I add in before checkin? It was added for potential performance gain if users happen to mass delete downloads from Library. But more often than not, no/few downloads will be deleted in the transaction because most items are pages without downloads.

It doesn't seem like Expire starts/ends transactions before expiring pages, and OnClearHistory just uses CleanUp.

I suppose if we back out everything, should we just disable the downlaod expiration test?
http://hg.mozilla.org/mozilla-central/index.cgi/rev/5e8fa7e3f219#l8.41

Comment 17

[Carsten Book [:Tomcat]]

Note: also crashed with the steps to reproduce from comment#13 also again, but this time with a different stack - filed Bug 431597 for this

Comment 18

[Marcia Knous [:marcia]]

Tomcat asked me to test this on my machine, I crashed when importing all data from Safari: my report ID is http://crash-stats.mozilla.com/report/index/7434c74c-170a-11dd-aa2e-001cc45a2c28. It is interesting that the top stack is in nsDownload Manager. My Build ID is  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008043004 Minefield/3.0pre.

Comment 19

[Ed Lee :Mardak]

Attachment: v1

mm smart pointers mmm

Comment 20

[Shawn Wilsher :sdwilsh]

Comment on attachment 318747 [details] [diff] [review]
v1

ugh - I thought this *was* a smart pointer.  Shame on me for missing that the first time around.

r=sdwilsh

Comment 21

[Ed Lee :Mardak]

Comment on attachment 318747 [details] [diff] [review]
v1

Here's a build for people to try:
https://build.mozilla.org/tryserver-builds/2008-04-30_21:[e-mail]/

Comment 22

[Tony Chung [:tchung]]

I've verified the safari data importing now works and does not crash anymore on the tryserver build provided here.  Thanks Mardak!  

Also, i can't repro the steps to crash from comment 13. 

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008043021 Minefield/3.0pre.

Comment 23

[Carsten Book [:Tomcat]]

(In reply to comment #22)
> I've verified the safari data importing now works and does not crash anymore on
> the tryserver build provided here.  Thanks Mardak!  
> 
> Also, i can't repro the steps to crash from comment 13. 
> 
Confirmed, the Tryserver Build fixes my crashes, also the crash from Bug 431597, thanks guys !

Comment 24

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 318747 [details] [diff] [review]
v1

a=shaver, please proceed to target with all haste.

Comment 25

[Ed Lee :Mardak]

http://hg.mozilla.org/cvs-trunk-mirror/index.cgi/rev/f29989c8508c
http://hg.mozilla.org/mozilla-central/index.cgi/rev/f29989c8508c

Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.185; previous revision: 1.184
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.67; previous revision: 1.66
done
Checking in toolkit/components/downloads/test/unit/test_history_expiration.js;
/cvsroot/mozilla/toolkit/components/downloads/test/unit/test_history_expiration.js,v  <--  test_history_expiration.js
new revision: 1.4; previous revision: 1.3
done

Comment 26

[Marcia Knous [:marcia]]

I was crashing on Win XP importing IE data, but using the Tryserver Build in Comment 21 I don't get a crash. Cool.

Comment 27

[Jim Jeffery not reading bug-mail 1/2/11]

No crash, Verified using Vista HP SP1 and the latest hourly:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050114 Minefield/3.0pre Firefox/2.0.0.14 ID:2008050114

Comment 28

[Carsten Book [:Tomcat]]

verified fixed per comment #26/27 and my own testing, thanks everyone for fixing this so fast !