485442 - crash bug calling nsINavHistoryQuery.uri

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[David Dahl :ddahl]

Working on bug 416580, I experienced a reproducible crash.

In my javascript shell I was doing the following:

var a = PlacesOrganizer._places.getSelectionNodes();
var b = a[0]; // b is an '[xpconnect wrapped (nsISupports, nsINavHistoryResultNode, nsINavHistoryContainerResultNode, nsINavHistoryQueryResultNode)]'
var q2 = b.getQueries(); // q2 is an [xpconnect wrapped nsINavHistoryQuery]
var uri = q2[0].uri; // crash with crash reporter!!

Comment 1

[David Dahl :ddahl]

Attachment: test case that doesn't seem run - it just dies

I also tried to write an XPC Shell test, but it uses different code paths and does not crash the browser, weird

Comment 2

[Marco Bonardo [:mak]]

to check that you should use nsINavHistoryQuery.hasUri.
on the other side i can't see why GetURI doesn't check if mURI is null. should probably be NS_IF_ADDREF instead of NS_ADDREF in nsNavHistoryQuery::GetUri, could you check if changing that is still crashing and eventually attach a patch?

Comment 3

[Shawn Wilsher :sdwilsh]

wait, why would mURI be null?

Comment 4

[Marco Bonardo [:mak]]

because the query could not be querying on uri

Comment 5

[David Dahl :ddahl]

with NS_IF_ADDREF, still segfaulting - I will re-run this test once my debug build is updated.

Comment 6

[David Dahl :ddahl]

Attachment: test case and patch (sorry it is a browser test)

added the browser test to browser/components/places/tests/browser as the toolkit browser tests are hosed

Comment 7

[David Dahl :ddahl]

I will write an xpcshell testcase and then ask for review.

Comment 8

[David Dahl :ddahl]

My stack trace before applying my patch:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x411d88f0 (LWP 10974)]
0x41d0bd16 in nsNavHistoryQuery::GetUri (this=0xa072cc8, aUri=0xbf9cbf24)
    at /home/ddahl/code/moz/mozilla-central/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp:1073
1073	  NS_ADDREF(*aUri);
(gdb) bt
#0  0x41d0bd16 in nsNavHistoryQuery::GetUri (this=0xa072cc8, aUri=0xbf9cbf24)
    at /home/ddahl/code/moz/mozilla-central/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp:1073
#1  0x406fec4f in NS_InvokeByIndex_P ()
   from /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/dist/bin/libxpcom_core.so
#2  0x413d206d in XPCWrappedNative::CallMethod (ccx=@0xbf9cc200, mode=XPCWrappedNative::CALL_GETTER)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2480
#3  0x413e33ff in XPCWrappedNative::GetAttribute (ccx=@0xbf9cc200)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/src/xpcprivate.h:2322
#4  0x413ddae0 in XPC_WN_GetterSetter (cx=0x9fd6280, obj=0xa174060, argc=0, argv=0xa0305e0, vp=0xbf9cc30c)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1617
#5  0x4015e7f8 in js_Invoke (cx=0x9fd6280, argc=0, vp=0xa0305d8, flags=2)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsinterp.cpp:1380
#6  0x4015f053 in js_InternalInvoke (cx=0x9fd6280, obj=0xa174060, fval=169296416, flags=0, argc=0, argv=0x0, 
    rval=0xbf9ccb74) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsinterp.cpp:1441
#7  0x4015f2cd in js_InternalGetOrSet (cx=0x9fd6280, obj=0xa174060, id=167705460, fval=169296416, 
    mode=JSACC_READ, argc=0, argv=0x0, rval=0xbf9ccb74)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsinterp.cpp:1504
#8  0x4016f91f in js_GetSprop (cx=0x9fd6280, sprop=0xa087ff8, obj=0xa174060, vp=0xbf9ccb74)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsscope.h:359
#9  0x40170307 in js_NativeGet (cx=0x9fd6280, obj=0xa174060, pobj=0xa174060, sprop=0xa087ff8, vp=0xbf9ccb74)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsobj.cpp:4142
#10 0x40177d66 in js_GetPropertyHelper (cx=0x9fd6280, obj=0xa174060, id=167705460, vp=0xbf9ccb74, 
    entryp=0xbf9cc8fc) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsobj.cpp:4306
#11 0x40146102 in js_Interpret (cx=0x9fd6280)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsinterp.cpp:4378
#12 0x4015dc2f in js_Execute (cx=0x9fd6280, chain=0x9ff31a0, script=0xa0a5a18, down=0x0, flags=0, 
    result=0xbf9ccd54) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsinterp.cpp:1614

Comment 9

[David Dahl :ddahl]

Whoops I missed the end of this trace:


Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x411d88f0 (LWP 10974)]
0x41d0bd16 in nsNavHistoryQuery::GetUri (this=0xa072cc8, aUri=0xbf9cbf24)
    at /home/ddahl/code/moz/mozilla-central/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp:1073
1073	  NS_ADDREF(*aUri);
(gdb) bt
#0  0x41d0bd16 in nsNavHistoryQuery::GetUri (this=0xa072cc8, aUri=0xbf9cbf24)
    at /home/ddahl/code/moz/mozilla-central/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp:1073
#1  0x406fec4f in NS_InvokeByIndex_P ()
   from /home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-debug/dist/bin/libxpcom_core.so
#2  0x413d206d in XPCWrappedNative::CallMethod (ccx=@0xbf9cc200, mode=XPCWrappedNative::CALL_GETTER)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2480
#3  0x413e33ff in XPCWrappedNative::GetAttribute (ccx=@0xbf9cc200)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/src/xpcprivate.h:2322
#4  0x413ddae0 in XPC_WN_GetterSetter (cx=0x9fd6280, obj=0xa174060, argc=0, argv=0xa0305e0, vp=0xbf9cc30c)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1617
#5  0x4015e7f8 in js_Invoke (cx=0x9fd6280, argc=0, vp=0xa0305d8, flags=2)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsinterp.cpp:1380
#6  0x4015f053 in js_InternalInvoke (cx=0x9fd6280, obj=0xa174060, fval=169296416, flags=0, argc=0, argv=0x0, 
    rval=0xbf9ccb74) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsinterp.cpp:1441
#7  0x4015f2cd in js_InternalGetOrSet (cx=0x9fd6280, obj=0xa174060, id=167705460, fval=169296416, 
    mode=JSACC_READ, argc=0, argv=0x0, rval=0xbf9ccb74)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsinterp.cpp:1504
#8  0x4016f91f in js_GetSprop (cx=0x9fd6280, sprop=0xa087ff8, obj=0xa174060, vp=0xbf9ccb74)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsscope.h:359
#9  0x40170307 in js_NativeGet (cx=0x9fd6280, obj=0xa174060, pobj=0xa174060, sprop=0xa087ff8, vp=0xbf9ccb74)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsobj.cpp:4142
#10 0x40177d66 in js_GetPropertyHelper (cx=0x9fd6280, obj=0xa174060, id=167705460, vp=0xbf9ccb74, 
    entryp=0xbf9cc8fc) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsobj.cpp:4306
#11 0x40146102 in js_Interpret (cx=0x9fd6280)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsinterp.cpp:4378
#12 0x4015dc2f in js_Execute (cx=0x9fd6280, chain=0x9ff31a0, script=0xa0a5a18, down=0x0, flags=0, 
    result=0xbf9ccd54) at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsinterp.cpp:1614
---Type <return> to continue, or q <return> to quit---
#13 0x400d68e0 in JS_ExecuteScript (cx=0x9fd6280, obj=0x9ff31a0, script=0xa0a5a18, rval=0xbf9ccd54)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/jsapi.cpp:5073
#14 0x0804ca2a in ProcessFile (cx=0x9fd6280, obj=0x9ff31a0, filename=0x0, file=0x40632420, forceTTY=1)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/shell/xpcshell.cpp:904
#15 0x0804cc1f in Process (cx=0x9fd6280, obj=0x9ff31a0, filename=0x0, forceTTY=1)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/shell/xpcshell.cpp:943
#16 0x0804d21b in ProcessArgs (cx=0x9fd6280, obj=0x9ff31a0, argv=0xbf9cdfe0, argc=17)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/shell/xpcshell.cpp:1108
#17 0x0804dd6e in main (argc=17, argv=0xbf9cdfe0, envp=0xbf9ce028)
    at /home/ddahl/code/moz/mozilla-central/mozilla/js/src/xpconnect/shell/xpcshell.cpp:1747

Comment 10

[David Dahl :ddahl]

Attachment: xpc shell test, browser test and patch

Comment 11

[David Dahl :ddahl]

BTW:

These tests are under browser/components/places/tests for now - I can move the xpcshell test to toolkit if you want (sdwilsh? mak77?)

Comment 12

[Marco Bonardo [:mak]]

yes please, if crash is reproduceable in xpcshell test without your patch, then remove browser-chrome test and move xpcshell to toolkit/unit

PS: test_485442_crash_bug_nsiNavHistoryNode.js should probably be called test_485442_crash_bug_nsNavHistoryQuery_GetUri.js

Comment 13

[David Dahl :ddahl]

Attachment: removed browser test, moved xpc shell test

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 372930 [details] [diff] [review]
removed browser test, moved xpc shell test

>diff --git a/toolkit/components/places/tests/unit/test_485442_crash_bug_nsNavHistoryQuery_GetUri.js b/toolkit/components/places/tests/unit/test_485442_crash_bug_nsNavHistoryQuery_GetUri.js
>+
>+// Delete a previously created sqlite file
>+function clearDB() {
>+  try {
>+    var file = dirSvc.get('ProfD', Ci.nsIFile);
>+    file.append("places.sqlite");
>+    if (file.exists())
>+      file.remove(false);
>+  } catch(ex) { dump("Exception: " + ex); }
>+}
>+clearDB();

what's the purpose of removing the db at test start? should not be needed by a test that does not add anything to it, please remove this.

>+
>+var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>+         getService(Ci.nsINavHistoryService);
>+
>+function run_test() {
>+

nit: useless newline

>+  var query = hs.getNewQuery();
>+  var options = hs.getNewQueryOptions();
>+  options.resultType = options.RESULT_TYPE_QUERY;
>+  var result = hs.executeQuery(query, options);
>+  result.root.containerOpen = true;
>+  var c = result.root;

call this rootNode, c is meaningless

>+  c.QueryInterface(Ci.nsINavHistoryQueryResultNode);
>+  var q = {};
>+  var d = c.getQueries(q);

make this var queries = rootNode.getQueries({});

>+  do_check_eq(d[0].uri, null); // Should be null, instead of crashing the browser
>+
>+}

also please close container before exiting.

r=me with those fixed, thanks!

Comment 15

[David Dahl :ddahl]

Attachment: updated patch

This was still crashing today, with fix applied and after rebuilding toolkit/ entirely. sdwilsh had me change the NS_IF_ADDREF call like this: NS_IF_ADDREF(*aUri = mUri);

Now the updated test passes.

Asking for review again because of this change.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 373156 [details] [diff] [review]
updated patch

r=sdwilsh

Comment 17

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/49ba14fac398

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 373156 [details] [diff] [review]
updated patch

asking approval 1.9.1, practically no risk.
Removes a case where an add-on could cause a browser crash.

Comment 19

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

Comment on attachment 373156 [details] [diff] [review]
updated patch

a191=beltzner

Comment 20

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/63a9ba2a1bc1

Comment 21

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv