Closed Bug 153480 Opened 19 years ago Closed 19 years ago

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

Categories

(Core Graveyard :: RDF, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.1beta

People

(Reporter: jrgmorrison, Assigned: tingley)

References

()

Details

(Keywords: crash, testcase, topcrash+, Whiteboard: [adt2 rtm])

Crash Data

Attachments

(5 files, 2 obsolete files)

("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 aha@pinknet.cz 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)
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. 

Severity: normal → critical
Keywords: crash
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=""/>
Keywords: nsbeta1
Adding topcrash+ and testcase keywords.  This is a topcrasher on the
MozillaTrunk and first started showing up with builds from 6/20.
Keywords: testcase, topcrash+
*** Bug 153866 has been marked as a duplicate of this bug. ***
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.)
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.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.1beta
This crash is reproducible on the branch as well, to be clear.
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.
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.
Adding Trunk and M1BR to summary since this is crashing both the MozillaTrunk
and Gecko1.0 Branch.
Summary: crash deleting download manager entries [@ InMemoryArcsEnumeratorImpl::HasMoreElements] → crash deleting download manager entries Trunk M1BR [@ InMemoryArcsEnumeratorImpl::HasMoreElements]
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.
Someone in the talkback comments noted that this crash happened after removing
the last item.
Keywords: adt1.0.1
jrgm: regarding the about="", might be related to
http://bugzilla.mozilla.org/show_bug.cgi?id=132905#c45 ?
*** Bug 155252 has been marked as a duplicate of this bug. ***
waterson, any idea what's going on here?
removing adt1.0.1 until there is a patch with sr=/r=
Keywords: adt1.0.1
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.
->all, as i've been seeing this quite a lot on Mac OS X (at least with my branch
builds of late).
OS: Windows 2000 → All
Hardware: PC → All
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. 
To clarify, by "sane" I mean, "sane to the RDF module".  It may still be causing
the download manager to go berserk.
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).
Actually, nsInMemoryDataSource.cpp, line 783, is where I crash as well 
(|this| being [suddenly] null).
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.
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 on attachment 90053 [details] [diff] [review]
tingley's oh-so-close patch (-c format)

Rats.  Looking into it...
Attachment #90053 - Attachment is obsolete: true
I found the problem.  I'll upload a new patch sometime in the next
few hours.
You're a star, tingley!
Assignee: waterson → tingley
Status: ASSIGNED → NEW
Attached patch better version (obsolete) — Splinter Review
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.
Attached patch better versionSplinter Review
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 on attachment 90115 [details] [diff] [review]
better version

obsolete the duplicate attachment
Attachment #90115 - Attachment is obsolete: true
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 on attachment 90116 [details] [diff] [review]
better version

sr=waterson
Attachment #90116 - Flags: superreview+
Feel free to clean up the squirrely

  else
  while (...) {
  }

indentation, too. :-)
*** Bug 153952 has been marked as a duplicate of this bug. ***
*** Bug 153724 has been marked as a duplicate of this bug. ***
*** Bug 155799 has been marked as a duplicate of this bug. ***
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 :-)
*** Bug 156046 has been marked as a duplicate of this bug. ***
Comment on attachment 90116 [details] [diff] [review]
better version

r=rjc	Great detective work, Chase.
Attachment #90116 - Flags: review+
Whiteboard: [adt2 rtm]
Status: NEW → ASSIGNED
Fix checked in.  Thanks a ton, jrgm.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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).
Status: RESOLVED → VERIFIED
(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 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
Attachment #90116 - Flags: approval+
adding adt1.0.1+.  Please check this into the branch today if possible.
Keywords: adt1.0.1adt1.0.1+
Checked in to the branch.
Keywords: fixed1.0.1
*** Bug 158051 has been marked as a duplicate of this bug. ***
*** Bug 156494 has been marked as a duplicate of this bug. ***
adding verified1.0.1
Keywords: verified1.0.1
Crash Signature: [@ InMemoryArcsEnumeratorImpl::HasMoreElements]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.