153480 - crash deleting download manager entries Trunk M1BR [@ InMemoryArcsEnumeratorImpl::HasMoreElements]

Status: VERIFIED FIXED

(Core Graveyard :: RDF, defect, P1)

Description

[John Morrison]

("Oh", he thought. "What could possibly go wrong.").

I noticed that in the trunk builds for Jun 20, after the checkin for bug 142824
("Downloads.rdf keeps on growing") which changed the code in RemoveDownloads
to clean up the resources from downloads.rdf, that, hrm, one of the new 
topcrashers was the following stack. 

However, it's a little skewed as a topcrash entry, since 16 of the 18 entries 
are from [e-mail] who duly submitted the entries :-). What would 
help in getting this resolved would be if Adam could provide a copy of the
downloads.rdf that is crashing for him. Is that possible (either as an attachment
or maybe better as private email)? That should make it pretty straight forward
to duplicate this crash in a debugger.

InMemoryArcsEnumeratorImpl::HasMoreElements
[c:/builds/seamonkey/mozilla/rdf/base/src/nsInMemoryDataSource.cpp, line 783]
nsDownloadManager::RemoveDownload
[c:/builds/seamonkey/mozilla/xpfe/components/download-manager/src/nsDownloadManager.cpp,
line 666]
XPTC_InvokeByIndex
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 106]
XPCWrappedNative::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 1996]
XPC_WN_CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1267]
js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 790]
js_Interpret [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 2744]
js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 806]
nsXPCWrappedJSClass::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp, line 1195]
nsXPCWrappedJS::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp, line 430]
PrepareAndDispatch
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp,
line 117]
SharedStub
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp,
line 139]
XPTC_InvokeByIndex
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 106]
XPCWrappedNative::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 1996]
XPC_WN_CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1267]
js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 790]
js_Interpret [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 2744]
js_Invoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 806]
js_InternalInvoke [c:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 881]
JS_CallFunctionValue [c:/builds/seamonkey/mozilla/js/src/jsapi.c, line 3430]
nsJSContext::CallEventHandler
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1045]
nsJSEventListener::HandleEvent
[c:/builds/seamonkey/mozilla/dom/src/events/nsJSEventListener.cpp, line 184]
nsEventListenerManager::HandleEventSubType
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp, line
1222]
nsEventListenerManager::HandleEvent
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp, line
2221]
nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp, line 3447]
PresShell::HandleDOMEventWithTarget
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6216]
nsButtonBoxFrame::MouseClicked
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsButtonBoxFrame.cpp, line 192]
nsButtonBoxFrame::HandleEvent
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsButtonBoxFrame.cpp, line 142]
PresShell::HandleEventInternal
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6185]
PresShell::HandleEventWithTarget
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6136]
nsEventStateManager::CheckForAndDispatchClick
[c:/builds/seamonkey/mozilla/content/events/src/nsEventStateManager.cpp, line 2751]
nsEventStateManager::PostHandleEvent
[c:/builds/seamonkey/mozilla/content/events/src/nsEventStateManager.cpp, line 1757]
PresShell::HandleEventInternal
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6189]
PresShell::HandleEvent
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp, line 6091]
nsViewManager::HandleEvent
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 2085]
nsView::HandleEvent [c:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 306]
nsViewManager::DispatchEvent
[c:/builds/seamonkey/mozilla/view/src/nsViewManager.cpp, line 1896]
HandleEvent [c:/builds/seamonkey/mozilla/view/src/nsView.cpp, line 83]
nsWindow::DispatchEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1029]
nsWindow::DispatchWindowEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1046]
nsWindow::DispatchMouseEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 4932]
ChildWindow::DispatchMouseEvent
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 5187]
nsWindow::ProcessMessage
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 3759]
nsWindow::WindowProc
[c:/builds/seamonkey/mozilla/widget/src/windows/nsWindow.cpp, line 1291]
USER32.DLL + 0x2e98 (0x77e12e98)
USER32.DLL + 0x30e0 (0x77e130e0)
USER32.DLL + 0x5824 (0x77e15824)
nsAppShellService::Run
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 458]
main1 [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1472]
main [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1808]
WinMain [c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1826]
WinMainCRTStartup()
KERNEL32.DLL + 0x17d08 (0x77e97d08)

Comment 1

[Adam Hauner]

Attachment: Adam's download.rdf (zipped)

Comment 2

[Adam Hauner]

Additional info: 
I have several weeks (still using actual nightbuild) problems with saving files
with czech national chars (filled as bug 148934) - browser was crashing after
Download dialog, some content of file was saved, but not all. When I wanna
remove crashed file from Download Manager, browser crashed every time. So I
generate topcrasher =)

In mentioned bug are several JS errors I got while operating with same record in
Download manager, before operations leads to crash browser. 

Comment 3

[John Morrison]

Adam. Thanks a lot for attaching this.

A quick look at that file shows that, while well-formed, it doesn't look 
correct (independent of i18n issues). There is a  RDF:Description in 
there that has a blank |about=""|, and then in the RDF:Seq there are 
multiple references to this unnamed resource. (Okay, I could be unaware 
that this is a valid format for that file, but I doubt it.)

  <RDF:Seq about="NC:DownloadsRoot">
    <RDF:li resource=""/>
    <RDF:li resource=""/>
    <RDF:li resource=""/>

Comment 4

[Jay Patel [:jay]]

Adding topcrash+ and testcase keywords.  This is a topcrasher on the
MozillaTrunk and first started showing up with builds from 6/20.

Comment 5

[Jay Patel [:jay]]

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

Comment 6

[John Morrison]

Here's one reduced testcase of downloads.rdf that crashes. This may not (is
likely not) the only possible way that the attached downloads.rdf crashes.

<?xml version="1.0"?>
<RDF:RDF xmlns:NC="http://home.netscape.com/NC-rdf#"
         xmlns:RDF="http://www.w3.org/1999/02/22-rdf-syntax-ns#">
  <RDF:Description about="">
    <NC:Name>foobar.html</NC:Name>
    <NC:Name>whatever.html</NC:Name>
  </RDF:Description>
  <RDF:Seq about="NC:DownloadsRoot">
    <RDF:li resource=""/>
    <RDF:li resource="blahblah.html"/>
  </RDF:Seq>
</RDF:RDF>

If you drop that in as a replacement for downloads.rdf in a profile, it will
crash in HasMoreElements() with |this| being null.

While the above may be a broken structure for that file, it (or equivalent)
was somehow written into that file.

I wonder if one way to "fix" this bug for the case of nsDownloadManager is 
to add an assertion in downloads.rdf that flags the file as being created
after some release version. In other words, if we see a downloads.rdf that
was created before we began cleaning up as we go along (and other fixes), 
just blow the old one away, create a new downloads.rdf and then flag this new 
file as 'valid' for future use. (This is based on a guess that this sort of 
mangled file structure may be the result of using the download manager in its 
earlier forms and we won't likely see this in downloads.rdf created after a 
certain date. But that's a guess.)

Comment 7

[John Morrison]

By the way, Nagi Peters, it would help (if you're willing) to provide the 
crashing downloads.rdf from duplicate bug 153866, either by attaching to this
bug report or emailing it to myself or blaker.

Comment 8

[John Morrison]

This crash is reproducible on the branch as well, to be clear.

Comment 9

[Nagi Peters]

Attachment: My Download.rdf Crashed by removing indéex.php3.html

My download.rdf the file mentioned in bug 153866 is lost, but it is also
reprocible with indéex.php3.html (indéx.php3.html) in this file.

Comment 10

[John Morrison]

Seems to largely the same sort of problem (i.e., the resource with 'about=""'
that has collected a bunch of unrelated assertions. I also found this other, 
similar, crash case.

<?xml version="1.0"?>
<RDF:RDF xmlns:NC="http://home.netscape.com/NC-rdf#"
         xmlns:RDF="http://www.w3.org/1999/02/22-rdf-syntax-ns#">
  <RDF:Description about="file:///C:/whatever.exe"
                   NC:Name="whatever.exe"
                   NC:ProgressMode="none"
                   NC:StatusText="Finished">
    <NC:Transferred>2268KB of 2268KB</NC:Transferred>
    <NC:Transferred>2355KB of 2355KB</NC:Transferred>
    <NC:URL resource="http://whatever.com/whatever.exe"/>
    <NC:File resource="file:///C:/whatever.exe"/>
  </RDF:Description>
  <RDF:Seq about="NC:DownloadsRoot">
    <RDF:li resource="file:///C:/whatever.exe"/>
  </RDF:Seq>
</RDF:RDF>

Note: I have flattened all the 8-bit characters to 7bit and I still crash, so
I doubt this has (much) to do with the character set.

Comment 11

[Jay Patel [:jay]]

Adding Trunk and M1BR to summary since this is crashing both the MozillaTrunk
and Gecko1.0 Branch.

Comment 12

[Jarmo]

Dont know if this helps, but. It seems that duplicated entries in DESCRIPTION
section causes crash if deleted. Ive one that have two <NC:URL resource> tags,
and if I try to delete the row (from download manager) Mozilla crashes. If I
remove one of the URL tags, mozilla behavies. Let me prove it... see no crash
:-) the same "downloads.rdf", but now without duplicate. Let me know if you want
the file.

Comment 13

[Blake Ross]

Someone in the talkback comments noted that this crash happened after removing
the last item.

Comment 14

[Peter Van der Beken [:peterv]]

jrgm: regarding the about="", might be related to
http://bugzilla.mozilla.org/show_bug.cgi?id=132905#c45 ?

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Blake Ross]

waterson, any idea what's going on here?

Comment 17

[Paul Wyskoczka]

removing adt1.0.1 until there is a patch with sr=/r=

Comment 18

[Christian Eyrich]

Can confirm this with Builds 20020623 and 2002070204 on Linux.
No special characters or empty about-lines in the rdf.
Talkback ID: TB7934399H
Downloads.rdf for testing follows.

Comment 19

[Christian Eyrich]

Attachment: Christians Downloads.rdf

Comment 20

[sairuh (rarely reading bugmail)]

->all, as i've been seeing this quite a lot on Mac OS X (at least with my branch
builds of late).

Comment 21

[Chase Tingley]

Regarding comment #3 and others, |about=""| is a valid RDF idiom (interpreted as
the url of the document being parsed); the parser appears to treat |resource=""|
the same way.

Loading the snippet in comment #6 into a datasource produces something that (to
xpcshell inspection) appears sane. 

Comment 22

[Chase Tingley]

To clarify, by "sane" I mean, "sane to the RDF module".  It may still be causing
the download manager to go berserk.

Comment 23

[Chase Tingley]

Attachment: js code to reproduce crash in xpcshell

I take it back, I believe this is purely an RDF issue.	Attached is JS code
that produces the same crash I see in the download manager (it apes jrgm's
RDF-deletion code).  The crash I'm seeing is slightly different from what jrgm
reported, in that I'm dying at nsInMemoryDataSource.cpp:783,
|NS_ADDREF(mCurrent);|, where mCurrent is bad memory (0xd8d8d8d8).

Comment 24

[John Morrison]

Actually, nsInMemoryDataSource.cpp, line 783, is where I crash as well 
(|this| being [suddenly] null).

Comment 25

[Chase Tingley]

Cool.

This seems to have something to do with the two <NC:Name> properties, strangely
enough.  If I remove one of them, the crash seems to go away.  Also, somehow I
missed previously that if you dump the arcs in js, the arc loop hits the NC:Name
properties twice, which shouldn't happen.  I think something funny is going on
in the enumerator code.

Comment 26

[John Morrison]

tingley sent me a patch (to be attached) since bugzilla was acting up for
him. Here are his comments from email:

| This patch fixes the crash for me.
| 
| In the test case, the |mAssertion| pointer in the arc enumerator is
| going bad, because it points to an assertion with the same property
| as the "current" assertion (ie the one from which |mCurrent| was
| derived).  This means that it will be found by a GetTargets() call
| and subsequently removed from the datasource, which does nasty
| things to the assertion chain the enumerator is traversing.
|
| The patch addresses this issue by throwing an extra kink into the
| logic: in addition to checking the mAlreadyReturned array to see if
| we've spat out this property before, it will also skip over
| "similar" assertions (as defined by property match) when calculating
| the next value of mAssertion.
| 
| I think it will still be possible to cause crashes similar to this
| through careful manipulation of the RDF interfaces; this case is
| worth fixing anyways, since it's a pretty common operation.
| 
| I also changed |mCurrent| to |next| in the mAlreadyReturned check.
| I did this because |mCurrent| is, I think, incorrect -- it should
| always be null following a successful GetNext() call, and |next| is
| the value we really want to be sanity checking anyways.
| 
| There may be a more elegant way to solve this problem, but it's
| about a thousand degrees in my apartment right now and I'm not
| thinking too clearly.

---
I've tried the patch out in my current win2k trunk build and for the two
reduced testcase RDF fragments above, it eliminates the crash.

Unfortunately, after testing in a bunch of affected places with no
(apparent) problmes, and trying out the crash test cases and the
attached, full-fledged downloads.rdf files and not crashing, I got
back to try the simple case: download some files, restart the browser 
and then reopen the download manager.

With the patch, when I restart, the entries in DL Mgr are blank. The 
patch seems to alter the serialization of the downloads.rdf file.
For example, if I delete downloads.rdf, then start up and save the
first three links on the mozilla home page, I get these two different
serialized forms of downloads.rdf after I subsequently shutdown.

without the patch: 

<?xml version="1.0"?>
<RDF:RDF xmlns:NC="http://home.netscape.com/NC-rdf#"
         xmlns:RDF="http://www.w3.org/1999/02/22-rdf-syntax-ns#">
  <RDF:Seq about="NC:DownloadsRoot">
    <RDF:li resource="H:\mozorg.html"/>
    <RDF:li resource="H:\feedback.html"/>
    <RDF:li resource="H:\get-involved.html"/>
  </RDF:Seq>
  <RDF:Description about="H:\mozorg.html"
                   NC:Name="mozorg.html"
                   NC:ProgressMode="none"
                   NC:StatusText="Finished"
                   NC:Transferred="6KB of  6KB">
    <NC:URL resource="http://www.mozilla.org/mozorg.html"/>
    <NC:File resource="H:\mozorg.html"/>
    <NC:DownloadState NC:parseType="Integer">1</NC:DownloadState>
    <NC:ProgressPercent NC:parseType="Integer">100</NC:ProgressPercent>
  </RDF:Description>
... snip 2 similarly structured entries ...
</RDF:RDF>

with the patch:

<?xml version="1.0"?>
<RDF:RDF xmlns:NC="http://home.netscape.com/NC-rdf#"
         xmlns:RDF="http://www.w3.org/1999/02/22-rdf-syntax-ns#">
  <RDF:Seq about="NC:DownloadsRoot">
    <RDF:li resource="H:\mozorg.html"/>
    <RDF:li resource="H:\feedback.html"/>
    <RDF:li resource="H:\get-involved.html"/>
  </RDF:Seq>
  <RDF:Description about="H:\mozorg.html">
    <NC:URL resource="http://www.mozilla.org/mozorg.html"/>
    <NC:File resource="H:\mozorg.html"/>
    <NC:DownloadState NC:parseType="Integer">1</NC:DownloadState>
    <NC:ProgressPercent NC:parseType="Integer">100</NC:ProgressPercent>
  </RDF:Description>
... snip 2 similarly structured entries ...
</RDF:RDF>

Comment 27

[John Morrison]

Attachment: tingley's oh-so-close patch (-c format)

Comment 28

[Chase Tingley]

Comment on attachment 90053 [details] [diff] [review]
tingley's oh-so-close patch (-c format)

Rats.  Looking into it...

Comment 29

[Chase Tingley]

I found the problem.  I'll upload a new patch sometime in the next
few hours.

Comment 30

[Chris Waterson]

You're a star, tingley!

Comment 31

[Chase Tingley]

Attachment: better version

Ok, this is better, and cleaner code as well.

If jrgm can't make this puke, I'd like to get rjc and waterson for r/sr. 
Unfortunately, I'm leaving for DC in an hour and won't be back until Sunday --
since this is a P1/topcrasher, somebody else should probably check it in for me
if it survives review.

Comment 32

[Chase Tingley]

Attachment: better version

Ok, this is better, and cleaner code as well.

If jrgm can't make this puke, I'd like to get rjc and waterson for r/sr. 
Unfortunately, I'm leaving for DC in an hour and won't be back until Sunday --
since this is a P1/topcrasher, somebody else should probably check it in for me
if it survives review.

Comment 33

[John Morrison]

Comment on attachment 90115 [details] [diff] [review]
better version

obsolete the duplicate attachment

Comment 34

[John Morrison]

Okay. I've tried the version-2.0 patch in my current win2k trunk build.  For
the two reduced testcase RDF fragments above, and for the three attached
downloads.rdf files, it eliminates the crash.

It also correctly serializes and deserializes new downloads (i.e., they are
properly shown in the UI). When an entry is deleted from the UI, the
corresponding resource is deleted from downloads.rdf.

For other areas which call HasMoreElements: I've been through bookmarks
(adding, deleting, D&D'ing within bookmarks mgr, D&D-ing in and out of BM Mgr,
D&D to personal toolbar), opening and closing windows, the language panel in
prefs (and other prefs panels), the character coding menu item (under View),
reading mail, reading help... I did not note any problems (aside from one
known problem in help where it throws an unrelated JS exception). Everything
appeared to work correclty

[Sidenote (not really about this crash/patch): for the "well aged"
downloads.rdf files, like the first two attached to this bug report, even when
all visible entries are removed from the UI, we still "orphan" a fair number
of resource entries. i.e., they are still there in the downloads.rdf file.  It
would be interesting to know (in a separate bug report) if, after deleting the
downloads.rdf file, it is still possible to reproduce these 'orphaned'
entries. Perhaps now that we delete resources when they are removed from the
UI, we won't get in that state. But maybe there is some other situation that
is causing us to drop entries.]

So, anyways, patch tests well. Tingley rules! Ready for r=/sr= from
rjc/waterson.

Comment 35

[Chris Waterson]

Comment on attachment 90116 [details] [diff] [review]
better version

sr=waterson

Comment 36

[Chris Waterson]

Feel free to clean up the squirrely

  else
  while (...) {
  }

indentation, too. :-)

Comment 37

[Boris 'pi' Piwinger]

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

Comment 38

[Boris 'pi' Piwinger]

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

Comment 39

[John Morrison]

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

Comment 40

[Christos Cheretakis]

In 2002070508 things seem to be better than before, but still not corrected
completely.

When removing the first entry from the list, mozilla always crashed reproducibly.

When removing random entries from the list, some of them can be removed without
any problem, some of them cause mozilla to crash. These same entries ALWAYS
cause mozilla to crash, though it does not seem to be anything special about
them --or at least I can't see it :-)

Comment 41

[Matthias Versen [:Matti]]

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

Comment 42

[Robert John Churchill]

Comment on attachment 90116 [details] [diff] [review]
better version

r=rjc	Great detective work, Chase.

Comment 43

[Chase Tingley]

Fix checked in.  Thanks a ton, jrgm.

Comment 44

[John Morrison]

Thank you, Chase.

Okay. So marking verified. Crashes have disappeared from talkback data.
I put out a call to people in QA to look for any oddities w.r.t. RDF, 
and don't have any feedback that there are any problems. I'll confirm
tomorrow but this looks like it's good to go on the branch. I'll request
driver approval in a moment.

(Adding adt1.0.1 just "because"; it doesn't apply to Chase).

Comment 45

[John Morrison]

(I'll just note that comment #40 above was made before this patch was ever 
checked into the trunk, so the point is Moo!).

Comment 46

[chris hofmann]

Comment on attachment 90116 [details] [diff] [review]
better version

a=chofmann for 1.0.1.  add the fixed1.0.1 keyword after checking into the
branch

Comment 47

[scottputterman]

adding adt1.0.1+.  Please check this into the branch today if possible.

Comment 48

[Chase Tingley]

Checked in to the branch.

Comment 49

[Matthias Versen [:Matti]]

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

Comment 50

[alge]

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

Comment 51

[Tom Everingham]

adding verified1.0.1