342110 - crash when copying and pasting bookmark folder [@ nsBookmarksService::UpdateBookmarkForwardProxy]

Reporter

Description

•

18 years ago

Using latest branch 1.8 build...

Steps to reproduce:
1. Create a new folder on the Bookmarks Toolbar.
2. Add five bookmarks to the folder.
3. Right click > copy on the folder, then right click > paste on an empty area on the toolbar.

Results:
Crash.

Expected results:
No crash.

Phil Ringnalda has verified by local backout that this is a regression from bug 168411.

Relevant bits of the stack:
Thread 0 Crashed:
0   org.mozilla.firefox 	0x003a1188 nsBookmarksService::UpdateBookmarkForwardProxy(nsIRDFDataSource*, nsIRDFResource*) + 268
1   org.mozilla.firefox 	0x003a13b0 nsBookmarksService::Assert(nsIRDFResource*, nsIRDFResource*, nsIRDFNode*, int) + 180
2   libxpcom_core.dylib 	0x2c05eaec _XPTC_InvokeByIndex + 216
3   org.mozilla.firefox 	0x0047fe70 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 2524
4   org.mozilla.firefox 	0x004730b0 XPC_WN_CallMethod(JSContext*, JSObject*, unsigned, long*, long*) + 220
5   libmozjs.dylib      	0x2302d57c js_Invoke + 1808
6   libmozjs.dylib      	0x23035788 js_Interpret + 29200
7   libmozjs.dylib      	0x2302d5c0 js_Invoke + 1876
8   org.mozilla.firefox 	0x007564a0 nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) + 3260
9   libxpcom_core.dylib 	0x2c05e988 PrepareAndDispatch + 560
10  libxpcom_core.dylib 	0x2c05f32c SharedStub + 112
11  org.mozilla.firefox 	0x003fd90c nsTransactionManager::BeginTransaction(nsITransaction*) + 172
12  org.mozilla.firefox 	0x003fbe30 nsTransactionManager::DoTransaction(nsITransaction*) + 160
13  libxpcom_core.dylib 	0x2c05eaec _XPTC_InvokeByIndex + 216
14  org.mozilla.firefox 	0x0047fe70 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 2524
15  org.mozilla.firefox 	0x004730b0 XPC_WN_CallMethod(JSContext*, JSObject*, unsigned, long*, long*) + 220
16  libmozjs.dylib      	0x2302d57c js_Invoke + 1808
17  libmozjs.dylib      	0x23035788 js_Interpret + 29200
18  libmozjs.dylib      	0x2302d5c0 js_Invoke + 1876
19  org.mozilla.firefox 	0x007564a0 nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) + 3260
20  libxpcom_core.dylib 	0x2c05e988 PrepareAndDispatch + 560
21  libxpcom_core.dylib 	0x2c05f32c SharedStub + 112
22  libxpcom_core.dylib 	0x2c05eaec _XPTC_InvokeByIndex + 216
23  org.mozilla.firefox 	0x0047fe70 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 2524
24  org.mozilla.firefox 	0x004730b0 XPC_WN_CallMethod(JSContext*, JSObject*, unsigned, long*, long*) + 220
25  libmozjs.dylib      	0x2302d57c js_Invoke + 1808
26  libmozjs.dylib      	0x23035788 js_Interpret + 29200
27  libmozjs.dylib      	0x2302d5c0 js_Invoke + 1876
28  org.mozilla.firefox 	0x007564a0 nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, nsXPTMethodInfo const*, nsXPTCMiniVariant*) + 3260
29  libxpcom_core.dylib 	0x2c05e988 PrepareAndDispatch + 560
30  libxpcom_core.dylib 	0x2c05f32c SharedStub + 112
31  libxpcom_core.dylib 	0x2c05eaec _XPTC_InvokeByIndex + 216
32  org.mozilla.firefox 	0x0047fe70 XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) + 2524
33  org.mozilla.firefox 	0x004730b0 XPC_WN_CallMethod(JSContext*, JSObject*, unsigned, long*, long*) + 220
34  libmozjs.dylib      	0x2302d57c js_Invoke + 1808
35  libmozjs.dylib      	0x23035788 js_Interpret + 29200
36  libmozjs.dylib      	0x2302d5c0 js_Invoke + 1876
37  libmozjs.dylib      	0x2302d7cc js_InternalInvoke + 184
38  libmozjs.dylib      	0x230080f0 JS_CallFunctionValue + 44
39  org.mozilla.firefox 	0x0024e6a8 nsJSContext::CallEventHandler(JSObject*, JSObject*, unsigned, long*, long*) + 284
40  org.mozilla.firefox 	0x0059af54 nsJSEventListener::HandleEvent(nsIDOMEvent*) + 948
41  org.mozilla.firefox 	0x001cfc20 nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned, unsigned) + 580
42  org.mozilla.firefox 	0x001cff78 nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned, nsEventStatus*) + 772
43  org.mozilla.firefox 	0x0020a9e0 nsXULElement::HandleDOMEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, unsigned, nsEventStatus*) + 2888
44  org.mozilla.firefox 	0x0020a234 nsXULElement::HandleDOMEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, unsigned, nsEventStatus*) + 924
45  org.mozilla.firefox 	0x001500c0 PresShell::HandleDOMEventWithTarget(nsIContent*, nsEvent*, nsEventStatus*) + 112
46  org.mozilla.firefox 	0x005ca5ac nsMenuFrame::Execute(nsGUIEvent*) + 820
47  org.mozilla.firefox 	0x005c7770 nsMenuFrame::HandleEvent(nsPresContext*, nsGUIEvent*, nsEventStatus*) + 532
48  org.mozilla.firefox 	0x0014febc PresShell::HandleEventInternal(nsEvent*, nsIView*, unsigned, nsEventStatus*) + 1088
49  org.mozilla.firefox 	0x0014f968 PresShell::HandleEvent(nsIView*, nsGUIEvent*, nsEventStatus*, int, int&) + 1300
50  org.mozilla.firefox 	0x00215264 nsViewManager::HandleEvent(nsView*, nsGUIEvent*, int) + 792
51  org.mozilla.firefox 	0x00214428 nsViewManager::DispatchEvent(nsGUIEvent*, nsEventStatus*) + 3356
52  org.mozilla.firefox 	0x0053a974 ViewWrapper::GetInterface(nsID const&, void**) + 468
53  org.mozilla.firefox 	0x006d5bd8 nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) + 172
54  org.mozilla.firefox 	0x006d5c64 nsWindow::DispatchWindowEvent(nsGUIEvent&) + 32
55  org.mozilla.firefox 	0x006d5d28 nsWindow::DispatchMouseEvent(nsMouseEvent&) + 112
56  org.mozilla.firefox 	0x006d22c0 nsMacEventHandler::HandleMouseUpEvent(EventRecord&) + 200
57  org.mozilla.firefox 	0x006d0824 nsMacEventHandler::HandleOSEvent(EventRecord&) + 108
58  org.mozilla.firefox 	0x00325960 nsMacWindow::DispatchEvent(void*, int*) + 64
59  org.mozilla.firefox 	0x006ccd4c nsMacMessagePump::DispatchOSEventToRaptor(EventRecord&, OpaqueWindowPtr*) + 96
60  org.mozilla.firefox 	0x006ccc48 nsMacMessagePump::DoMouseUp(EventRecord&) + 76
61  org.mozilla.firefox 	0x006cc5d0 nsMacMessagePump::DispatchEvent(EventRecord*) + 92
62  org.mozilla.firefox 	0x006cce5c nsMacMessagePump::WNETransitionEventHandler(OpaqueEventHandlerCallRef*, OpaqueEventRef*, void*) + 44
63  com.apple.HIToolbox 	0x931d7794 DispatchEventToHandlers(EventTargetRec*, OpaqueEventRef*, HandlerCallRec*) + 692
64  com.apple.HIToolbox 	0x931d6eec SendEventToEventTargetInternal(OpaqueEventRef*, OpaqueEventTargetRef*, HandlerCallRec*) + 372
65  com.apple.HIToolbox 	0x931ddc8c SendEventToEventTarget + 40
66  com.apple.HIToolbox 	0x93269e58 HandleMouseEventForWindow(OpaqueWindowPtr*, OpaqueEventRef*, unsigned short) + 236
67  com.apple.HIToolbox 	0x933cc9c8 HandleMouseEvent(OpaqueEventRef*) + 368
68  com.apple.HIToolbox 	0x931ddff8 ToolboxEventDispatcherHandler(OpaqueEventHandlerCallRef*, OpaqueEventRef*, void*) + 496
69  com.apple.HIToolbox 	0x931d79e4 DispatchEventToHandlers(EventTargetRec*, OpaqueEventRef*, HandlerCallRec*) + 1284
70  com.apple.HIToolbox 	0x931d6eec SendEventToEventTargetInternal(OpaqueEventRef*, OpaqueEventTargetRef*, HandlerCallRec*) + 372
71  com.apple.HIToolbox 	0x931ddc8c SendEventToEventTarget + 40
72  com.apple.HIToolbox 	0x9321e9a0 ToolboxEventDispatcher + 92
73  com.apple.HIToolbox 	0x9321e92c HLTBEventDispatcher + 16
74  com.apple.HIToolbox 	0x9321cee4 RunApplicationEventLoop + 148
75  org.mozilla.firefox 	0x0031ef30 nsAppShell::Run() + 64
76  org.mozilla.firefox 	0x003b9acc nsAppStartup::Run() + 60
77  org.mozilla.firefox 	0x0001252c XRE_main + 3972
78  org.mozilla.firefox 	0x0000d768 start + 432
79  org.mozilla.firefox 	0x0000d5e8 start + 48

Adam Guthrie

Reporter

Comment 1

•

18 years ago

Attached file crashlog for non-wrapped viewing pleasure — Details

Phil Ringnalda (:philor)

Comment 2

•

18 years ago

Not Mac-only, and 5 isn't the magic number I thought it was, though oddly enough I can crash on Mac with just one bookmark in a folder, and on Linux I need two to crash.

Blocks: 168411

Flags: blocking-firefox2?

OS: Mac OS X 10.4 → All

Hardware: Macintosh → All

(not reading, please use seth@sspitzer.org instead)

Comment 3

•

18 years ago

I can reproduce this on mac, too.  investigating with jminta

Assignee: nobody → sspitzer

(not reading, please use seth@sspitzer.org instead)

Comment 4

•

18 years ago

in nsBookmarksService.cpp, like 373, urlLiteral is null.

the reason it is null is that urlNode is not a nsIRDFLiteral or nsIRDFNode.


from the code:

   // unfortunately, urlNode is a Literal; we need it to be a Resource
    nsCOMPtr<nsIRDFLiteral> urlLiteral = do_QueryInterface(urlNode);
    const PRUnichar *urlstr;
    rv = urlLiteral->GetValueConst(&urlstr);
    if (NS_FAILED(rv)) return rv;

when we crash, urlNode appears to be a js type object (forgive my ignorance!).  working with jminta to figure out what changed in js.

(not reading, please use seth@sspitzer.org instead)

Comment 5

•

18 years ago

Attached patch wallpaper, but not the fix (obsolete) — Details — Splinter Review

the following wallpaper prevents the crash, which is good, but the underlying problem is stilll there.  jminta and I are continuing to work on it...

Attachment #226437 - Flags: review?(jminta)

Joey Minta

Assignee

Comment 6

•

18 years ago

Comment on attachment 226437 [details] [diff] [review]
wallpaper, but not the fix

r=jminta with a -pU 8 in your .cvsrc :-)

Attachment #226437 - Flags: review?(jminta) → review+

(not reading, please use seth@sspitzer.org instead)

Comment 7

•

18 years ago

setting swag, as I'm sure this will get blocking status.

Status: NEW → ASSIGNED

Whiteboard: SWAG: 1d

Mike Connor [:mconnor]

Updated

•

18 years ago

Flags: blocking-firefox2? → blocking-firefox2+

Joey Minta

Assignee

Comment 8

•

18 years ago

It seems like something's already going wrong before we reach the actual transaction code.  I'm currently poking around at http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/content/bookmarks.js#1702
Prior to bug 168411, transaction.removedProp[i] looks like
removedProps is [xpconnect wrapped nsIRDFResource @ 0x8a5e218 (native @ 0x89d0668)],[xpconnect wrapped nsIRDFLiteral @ 0x8e2a390 (native @ 0x89d06b0)],,,,,,[xpconnect wrapped nsIRDFResource @ 0x8156ed8 (native @ 0x89d1320)],[xpconnect wrapped nsIRDFLiteral @ 0x81558b0 (native @ 0x89d1368)],[xpconnect wrapped nsIRDFLiteral @ 0x8b3ca68 (native @ 0x89d1258)],,,,,[xpconnect wrapped nsIRDFResource @ 0x89762f0 (native @ 0x89dc858)],[xpconnect wrapped nsIRDFLiteral @ 0x81558b0 (native @ 0x89d1368)],[xpconnect wrapped nsIRDFLiteral @ 0x8b3ca68 (native @ 0x89d1258)],,,,,[xpconnect wrapped nsIRDFResource @ 0x8b1e318 (native @ 0x89dccf0)],[xpconnect wrapped nsIRDFLiteral @ 0x81558b0 (native @ 0x89d1368)],[xpconnect wrapped nsIRDFLiteral @ 0x8b3ca68 (native @ 0x89d1258)],,,,,[xpconnect wrapped nsIRDFResource @ 0x8df7bf0 (native @ 0x89dcd38)],[xpconnect wrapped nsIRDFLiteral @ 0x81558b0 (native @ 0x89d1368)],[xpconnect wrapped nsIRDFLiteral @ 0x8b3ca68 (native @ 0x89d1258)],,,,,[xpconnect wrapped nsIRDFResource @ 0x835beb8 (native @ 0x89dcde8)],[xpconnect wrapped nsIRDFLiteral @ 0x81558b0 (native @ 0x89d1368)],[xpconnect wrapped nsIRDFLiteral @ 0x8b3ca68 (native @ 0x89d1258)],,,,

After, it looks like: 
removedProps is [xpconnect wrapped nsIRDFResource @ 0x8d4d758 (native @ 0x8961fc0)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x8d84430 (native @ 0x896f178)],,,,,,[xpconnect wrapped nsIRDFResource @ 0x89dd4d0 (native @ 0x896f270)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x89c61a0 (native @ 0x896f298)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x8a210e8 (native @ 0x896f1b8)],,,,,[xpconnect wrapped nsIRDFResource @ 0x89d0da8 (native @ 0x8978fe0)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x89c61a0 (native @ 0x896f298)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x8a210e8 (native @ 0x896f1b8)],,,,,[xpconnect wrapped nsIRDFResource @ 0x89efcf0 (native @ 0x896d4e8)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x89c61a0 (native @ 0x896f298)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x8a210e8 (native @ 0x896f1b8)],,,,,[xpconnect wrapped nsIRDFResource @ 0x89efe90 (native @ 0x896cb40)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x89c61a0 (native @ 0x896f298)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x8a210e8 (native @ 0x896f1b8)],,,,,[xpconnect wrapped nsIRDFResource @ 0x89d9718 (native @ 0x8978ef8)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x89c61a0 (native @ 0x896f298)],[xpconnect wrapped (nsISupports, nsIRDFNode, nsIRDFLiteral) @ 0x8a210e8 (native @ 0x896f1b8)],,,,
The first place these differ is at index [1] (notice the extra interfaces).  This is also where we're crashing.

Joey Minta

Assignee

Comment 9

•

18 years ago

I think I've got this one, but I want to talk it over with sspitzer to make sure I'm not overlooking anything.

Assignee: sspitzer → jminta

Status: ASSIGNED → NEW

Joey Minta

Assignee

Comment 10

•

18 years ago

Attached patch proper crash fixes — Details — Splinter Review

This patch contains fixes for a couple of problems.  First, removedProps should actually be nsIRDFLiteral in the idl, not nsIRDFNode.  Switching this solved the actual crash, however it pointed out problem #2, which was that I shouldn't have tried to be so minimal with the previous patch.

In the old bookmarks code (prior to bug 168411), the item in slot 0 of removedProps was an rdf resource and that slot was considered special.  After my patch, we no longer need that resource (we can use this.item).  Changing the idl caused problems because an rdf resource will not convert to an rdf literal.  Therefore, I also had to go and remove the spots where we were filling in slot 0 for removedProps.

Finally, there was a bug in copy-paste stemming from the fact that 168411 essentially removed 1 dimension of data for transactions (by forcing transactions to be done individually).  This is the reason for the changes in pasteBookmark.

This patch also includes sspitzer's bullet-proofing patch.

Attachment #226437 - Attachment is obsolete: true

Attachment #226802 - Flags: superreview?(mconnor)

Attachment #226802 - Flags: review?(sspitzer)

Mike Connor [:mconnor]

Updated

•

18 years ago

Attachment #226802 - Flags: superreview?(mconnor) → superreview+

(not reading, please use seth@sspitzer.org instead)

Comment 11

•

18 years ago

Comment on attachment 226802 [details] [diff] [review]
proper crash fixes

instead of:

propArray = new Array(6);

I'd suggest:

propArray = new Array(bkmkTxn.prototype.Properties.length);

(or whatever is the correct way to get that magic size, which is the number of properties)

two "please test" questions: is bug #315690 still fixed and, after delete, do deleted bookmarks show up in the search results (which vlad fixed with bug #255255)

Attachment #226802 - Flags: review?(sspitzer) → review+

(not reading, please use seth@sspitzer.org instead)

Comment 12

•

18 years ago

oops:

propArray = new Array(bkmkTxn.prototype.Properties.length);

I meant something like:

propArray = new Array(gBkmkTxnSvc.Properties.length);

Joey Minta

Assignee

Comment 13

•

18 years ago

(In reply to comment #12)
> I meant something like:
> propArray = new Array(gBkmkTxnSvc.Properties.length);
I think you really meant gBmProperties.length (since we're outside the txn-service and don't have access to those internal variables).

Patch landed on trunk.
Checking in browser/components/bookmarks/content/bookmarks.js;
/cvsroot/mozilla/browser/components/bookmarks/content/bookmarks.js,v  <--  bookmarks.js
new revision: 1.122; previous revision: 1.121
done
Checking in browser/components/bookmarks/public/nsIBookmarkTransactionManager.idl;
/cvsroot/mozilla/browser/components/bookmarks/public/nsIBookmarkTransactionManager.idl,v  <--  nsIBookmarkTransactionManager.idl
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/bookmarks/src/nsBookmarkTransactionManager.js;
/cvsroot/mozilla/browser/components/bookmarks/src/nsBookmarkTransactionManager.js,v  <--  nsBookmarkTransactionManager.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp;
/cvsroot/mozilla/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp,v  <--  nsBookmarksFeedHandler.cpp
new revision: 1.15; previous revision: 1.14
done
Checking in browser/components/bookmarks/src/nsBookmarksService.cpp;
/cvsroot/mozilla/browser/components/bookmarks/src/nsBookmarksService.cpp,v  <--  nsBookmarksService.cpp
new revision: 1.82; previous revision: 1.81
done

Joey Minta

Assignee

Updated

•

18 years ago

Status: NEW → RESOLVED

Closed: 18 years ago

Resolution: --- → FIXED

Joey Minta

Assignee

Comment 14

•

18 years ago

Comment on attachment 226802 [details] [diff] [review]
proper crash fixes

Asking for approval on this patch, since it fixes 2 regressions (this and bug 342116).  Additionally, baking on trunk is likely to produce little useful information as the patch would only be relevant if someone built without Places.

Attachment #226802 - Flags: approval1.8.1?

Mike Schroepfer

Updated

•

18 years ago

Attachment #226802 - Flags: approval1.8.1? → approval1.8.1+

Joey Minta

Assignee

Comment 15

•

18 years ago

Landed on 1.8 branch

Keywords: fixed1.8.1

Adam Guthrie

Reporter

Comment 16

•

18 years ago

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1a3) Gecko/20060628 BonEcho/2.0a3.

Copying and pasting a bookmarks folder with more than 5 bookmarks in it no longer crashes.

Status: RESOLVED → VERIFIED

Keywords: fixed1.8.1 → verified1.8.1

Whiteboard: SWAG: 1d

Nobody; OK to take it and work on it

Updated

•

13 years ago

Crash Signature: [@ nsBookmarksService::UpdateBookmarkForwardProxy]

crashlog for non-wrapped viewing pleasure 18 years ago Adam Guthrie 21.23 KB, text/plain		Details
wallpaper, but not the fix 18 years ago (not reading, please use seth@sspitzer.org instead) 939 bytes, patch	jminta : review+	Details \| Diff \| Splinter Review
proper crash fixes 18 years ago Joey Minta 11.25 KB, patch	moco : review+ mconnor : superreview+ mtschrep : approval1.8.1+	Details \| Diff \| Splinter Review