crash bug calling nsINavHistoryQuery.uri

RESOLVED FIXED in Firefox 3.6a1

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: ddahl, Assigned: ddahl)

Tracking

({crash, fixed1.9.1})

Trunk
Firefox 3.6a1
crash, fixed1.9.1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

9 years ago
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!!
(Assignee)

Comment 1

9 years ago
Created attachment 369595 [details]
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
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?
wait, why would mURI be null?
because the query could not be querying on uri
(Assignee)

Comment 5

9 years ago
with NS_IF_ADDREF, still segfaulting - I will re-run this test once my debug build is updated.
(Assignee)

Comment 6

9 years ago
Created attachment 372110 [details] [diff] [review]
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
Attachment #369595 - Attachment is obsolete: true
Attachment #372110 - Flags: review?
(Assignee)

Updated

9 years ago
Assignee: nobody → ddahl
(Assignee)

Updated

9 years ago
Attachment #372110 - Flags: review?
(Assignee)

Comment 7

9 years ago
I will write an xpcshell testcase and then ask for review.
(Assignee)

Comment 8

9 years ago
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
(Assignee)

Comment 9

9 years ago
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
(Assignee)

Comment 10

9 years ago
Created attachment 372744 [details] [diff] [review]
xpc shell test, browser test and patch
Attachment #372110 - Attachment is obsolete: true
Attachment #372744 - Flags: review?(mak77)
(Assignee)

Comment 11

9 years ago
BTW:

These tests are under browser/components/places/tests for now - I can move the xpcshell test to toolkit if you want (sdwilsh? mak77?)
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
(Assignee)

Comment 13

9 years ago
Created attachment 372930 [details] [diff] [review]
removed browser test, moved xpc shell test
Attachment #372744 - Attachment is obsolete: true
Attachment #372930 - Flags: review?(mak77)
Attachment #372744 - Flags: review?(mak77)
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!
Attachment #372930 - Flags: review?(mak77) → review+
(Assignee)

Comment 15

9 years ago
Created attachment 373156 [details] [diff] [review]
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.
Attachment #372930 - Attachment is obsolete: true
Attachment #373156 - Flags: review?(mak77)
Comment on attachment 373156 [details] [diff] [review]
updated patch

r=sdwilsh
Attachment #373156 - Flags: review?(mak77) → review+
http://hg.mozilla.org/mozilla-central/rev/49ba14fac398
Status: NEW → RESOLVED
Last Resolved: 9 years ago
OS: Linux → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.6a1
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.
Attachment #373156 - Flags: approval1.9.1?

Updated

9 years ago
Flags: in-testsuite+
Comment on attachment 373156 [details] [diff] [review]
updated patch

a191=beltzner
Attachment #373156 - Flags: approval1.9.1? → approval1.9.1+

Updated

9 years ago
Keywords: checkin-needed
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.