328113 - crash [@ nsIconChannel::GetName][@ nsMimeType::GetDescription][@ nsDownloadsDataSource::GetURI]

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Adam Guthrie]

From my e-mail to dveditz:

I was looking at talkback data for 1.5 and 1.5.0.1 and in 1.5.0.1 I
noticed that there was a topcrash with the top frame of
nsMimeType::GetDescription. So I checked out the stack and it looked
something like this:

nsMimeType::GetDescription
[c:/builds/tinderbox/Fx-Mozilla1.8.0/WINNT_5.2_Depend/mozilla/dom/src/base/nsMimeTypeArray.cpp,
line 269]
nsDownloadManager::~nsDownloadManager
[c:/builds/tinderbox/Fx-Mozilla1.8.0/WINNT_5.2_Depend/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,
line 147]
nsComponentManagerImpl::GetService
[c:/builds/tinderbox/Fx-Mozilla1.8.0/WINNT_5.2_Depend/mozilla/xpcom/components/nsComponentManager.cpp,
line 2105]
nsJSCID::GetService
[c:/builds/tinderbox/Fx-Mozilla1.8.0/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcjsid.cpp,
line 899]

Compare to:
nsIconChannel::GetName
[c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp,
line 113]
nsDownloadManager::~nsDownloadManager
[c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,
line 147]
nsComponentManagerImpl::GetServiceByContractID
[c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/xpcom/components/nsComponentManager.cpp,
line 2410]
CallGetService
[c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/xpcom/build/nsComponentManagerUtils.cpp,
line 95]

from bug 320065 that supposedly timeless's patch in bug 320498 will fix.
Anyway, there seems to be a core underlying issue here that has
something to do with the download manager. Lots of the comments from the
IDs say something about "downloading a file" or "opening the download
manager". Anyway, what I'm getting at is that I think someone with a bit
more knowledge than me needs to investigate this, seeing as how it still
seems to be affecting people in 1.5.0.1.

From my investigation:
The code that's being called in the nsDownloadManager destructor is this
line: gRDFService->UnregisterDataSource(mDataSource);. That leads to
this function:
http://lxr.mozilla.org/mozilla/source/rdf/base/src/nsRDFService.cpp#1400.
That calls various things, namely rv =
aDataSource->GetURI(getter_Copies(uri));, which leads to the two top
frames in both of the crashes mentioned above. timeless says we can fix
this by doing null checks in those functions, but this seems wrong to me
because depending on what type of download it is, there could be many
functions called to get the datasource's URI.

Comment 1

[Daniel Veditz [:dveditz]]

If a patch shows up and gets tested in time we could reconsider for 1.8.0.2 (ask for approval on the reviewed patch), but seems unlikely given the schedule.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow.  Those stacks look pretty completely bogus to me -- the download manager destructor does NOT call either of those two functions, and I see no reason the download manager would be getting destroyed from inside GetService().

That said, I wouldn't be surprised if there are various issues around hereabouts; if nothing else the way the forkage was done for download manager we have multiple classes with the same CID if someone manages to build both...

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The stack isn't as bogus as you think; talkback has trouble understanding it because many of the stack frames don't save the frame pointer on the stack; presumably when code jumps to garbage (or maybe even if it doesn't), talkback finds the stack by finding a coherent set of frames that point to each other.  I found exactly such a coherent set of frames matching what talkback said, and then read the disassembly of those functions (found by unpacking the 1.5.0.1 MAR and using objdump -Cd, on a Linux box), and found the stack is more reasonable than it looks.  Analysis to be posted shortly, once I finish.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

sorry, read the disassembly of the ones a little bit up the stack to work out the stack moving all the way to the bottom of the stack.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: my analysis of a raw stack dump

This analysis could probably be enhanced a bit more, by going back up and figuring out what the saved registers represent, and perhaps by doing something for the two words at the very bottom of the stack.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Note that this incident is for the Windows Fx 1.5.0.1 release.

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The top two words on the stack are consistent with nsDownloadsDataSource having a null mInner and the top frame being nsDownloadsDataSource::GetURI, but the bogus EIP is not consistent with that scenario.  Presumably talkback would have to get that bad EIP from somewhere, although perhaps a call dereferencing null could somehow mess with the EIP before crashing.

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: patch

...which is actually a perfectly possible crash scenario.  A failure at that point would leave mInner null, and would cause a crash exactly here (leaving the only mismatch with the data being talkback's indication of a bogus function at the top of the stack, which could come from some other source than EIP, I suppose, especially since the registers don't actually show up in the database.)

(That said, I'm not sure why UnregisterDataSource is needed at all.  I don't see any RegisterDataSource calls.)

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I'm also not sure if we should really be failing to create a download manager if we can't load the datasource...

Comment 10

[Brian Ryner (not reading)]

(In reply to comment #9)
> I'm also not sure if we should really be failing to create a download manager
> if we can't load the datasource...
> 

No, definitely not.  If the datasource is corrupt we should delete it and create a new one.

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

So, actually, RegisterDataSource is called by RDFXMLDataSourceImpl::Init.  Of course, there should probably be a bit of unregistering somewhere in the failure cases in nsRDFService::GetDataSourceBlocking and RDFXMLDataSourceImpl::Init...

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Though mrbkap couldn't find any type of corruption (bad XML, missing file) that actually caused the datasource to fail to load, and the code doesn't make it seem like a bad file would cause a failure nsresult there.

But there are cases that could cause such a failure nsresult; for example, if something else just started an asynchronous load of the same datasource.

Anyway, I could land this on trunk an 1.8.1 and see what happens; not sure if talkback is showing it there, though.

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I filed bug 330003 on comment 11.

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

This may well be the same as bug 320065, which was the same stack with a different unrelated function at the top.  There are comments there about corrupt downloads.rdf files (such that deleting the file would help).

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Patch checked in to trunk.

Comment 16

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Patch checked in to MOZILLA_1_8_BRANCH.

Comment 17

[Ryan Flint [:rflint] (ping via IRC for reviews)]

*** Bug 331393 has been marked as a duplicate of this bug. ***

Comment 18

[Peter]

The fix worked, Thanks!

I must say, I was very worried that something serious was wrong as I really like Firefox.  Keep on keppin' on!

Thanks,
Peter

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

*** Bug 320065 has been marked as a duplicate of this bug. ***

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 214643 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 21

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_8_0_BRANCH.

Comment 22

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

In Firefox 1.5.0.2, the function that appears as the talkback signature for this crash appears to be nsDownloadsDataSource::GetURI, which is, for the first time, the signature that appears for a related bug.

Comment 23

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

...which actually seems to be a valid stack for a change.

And also, this bug was originally reported, with a valid stack, and some more clues, as bug 280442.

Comment 24

[Adam Guthrie]

*** Bug 336929 has been marked as a duplicate of this bug. ***

Comment 25

[Jay Patel [:jay]]

v.fixed per latest Firefox15 Talkback data.  No crashes found for any of the stack signatures in this bug.  We should keep an eye on Talkback data post 1.5.0.4.