489872 - [@ nsNavHistory::IsVisited] doesn't check null aURI

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Ed Lee :Mardak]

Weave hits this when we fail to read out a uri, so when we create a uri with undefined, we end up passing null to isVisited.

Various other methods probably near there could make use of some NS_ENSURE_ARG_POINTER.

Comment 1

[Ed Lee :Mardak]

#3 result in the last week's crashes:

http://crash-stats.mozilla.com/query/query?query_type=startswith&query=nsNavHistory::&do_query=1

#2 is nsNavHistory::SetPageTitleInternal(nsIURI*, nsAString_internal const&)

that looks like null pointer too

Comment 2

[timeless]

it's a public entry point this should be fixed.

Comment 3

[Ed Lee :Mardak]

Attachment: output of v1 test

Comment 4

[Ed Lee :Mardak]

Attachment: v1

Add ENSURE_ARG_POINTER to all public entry points for nsNavHistory. Test automatically finds functions from nav history service and calls them with null arguments.

Comment 5

[Ed Lee :Mardak]

Attachment: v1.1

Updated test to use the amazingness of destructured function call args! :O :O :O

+  ].forEach(function([cid, iface, nothrow]) {

It's craaaaazzzyyy!

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 374383 [details] [diff] [review]
v1.1

NS_ENSURE_ARG should be used for in params, and NS_ENSURE_ARG_PTR for [in]out params.  I know they do the same check now, but we may end to do that differently in the future, so it's best to keep it right.

>diff --git a/toolkit/components/places/tests/unit/test_empty_tags.js b/toolkit/components/places/tests/unit/test_null_interfaces.js
>copy from toolkit/components/places/tests/unit/test_empty_tags.js
>copy to toolkit/components/places/tests/unit/test_null_interfaces.js
>--- a/toolkit/components/places/tests/unit/test_empty_tags.js
>+++ b/toolkit/components/places/tests/unit/test_null_interfaces.js
You did an hg copy that didn't really make sense.  Please just do an hg add of the new test.

Comment 7

[Ed Lee :Mardak]

(In reply to comment #6)
> (From update of attachment 374383 [details] [diff] [review])
> NS_ENSURE_ARG should be used for in params, and NS_ENSURE_ARG_PTR for [in]out
Should I fix up the existing NS_ENSURE_ARG_PTR or let something automatically fix it later perhaps bug 489914?

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#245
NS_ERROR_INVALID_ARG is used for truthiness as well as range checks. NS_ERROR_INVALID_POINTER seems more descriptive in our situation though.

> >diff --git a/toolkit/components/places/tests/unit/test_empty_tags.js b/toolkit/components/places/tests/unit/test_null_interfaces.js
> >--- a/toolkit/components/places/tests/unit/test_empty_tags.js
> >+++ b/toolkit/components/places/tests/unit/test_null_interfaces.js
> You did an hg copy that didn't really make sense
The only part that is shared is the license block. When you push to the repository, it doesn't show up as a copy anyway. But sure, I can just do a plain add.

Comment 8

[Shawn Wilsher :sdwilsh]

You can fix them now or not - I leave that up to you.  I'll take a patch either way.

It is an invalid argument to pass a null object, whereas it's an invalid pointer if the return value you pass is null.  It'd be really weird to get an exception about an invalid pointer in JS when you don't do anything with JS ;)

not that anybody checks exact result codes anyway...

Comment 9

[Ed Lee :Mardak]

Attachment: v2

(In reply to comment #8)
> It'd be really weird to get an exception about an invalid pointer in JS
Sounds reasonable. XPConnect actually throws NS_ERROR_XPC_NEED_OUT_OBJECT if you don't give a suitable out.

And interesting to note, NS_ERROR_ILLEGAL_VALUE and NS_ERROR_INVALID_ARG are the same.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 374406 [details] [diff] [review]
v2

Considering storage is the only thing that should ever call mozStorageFunctionGetUnreversedHost::OnFunctionCall, it should probably just be assertions so we don't do any more checks than we have to in release builds.  That, and have a unit test in storage to make sure that always happens...

BindStatementURI should just have NS_ASSERTION checks.  It's not a public API entry point, and we should assert that it's API misuse in debug builds.

Comments on the test momentarily.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 374406 [details] [diff] [review]
v2

>+let _ = function(some, debug, message) print(Array.slice(arguments).join(" "));
javadoc goodness please?  I figured out what this does by looking at the call sites, but it wasn't obvious at first, so a comment explaining what's going on will be useful for future folks looking at this test and wondering why it's suddenly failing.

>+function run_test() {
nit: brace on newline please

>+  _("Got an array of services to test");
>+  [["browser/nav-history-service;1", "nsINavHistoryService", ["queryStringToQueries", "removePagesByTimeframe", "removePagesFromHost"]],
nit: I'd rather have a global array of functions that we determine shouldn't throw.  It'll also make it easier to understand this test by having a descriptive variable name.

r=sdwilsh with comments addressed.

Comment 13

[Ed Lee :Mardak]

Attachment: v2.1

(In reply to comment #10)
> mozStorageFunctionGetUnreversedHost::OnFunctionCall, it should probably just be
> assertions so we don't do any more checks than we have to in release builds. 
> BindStatementURI should just have NS_ASSERTION checks
Added NS_ASSERTIONs to both

(In reply to comment #11)
> (From update of attachment 374406 [details] [diff] [review])
> javadoc goodness please?
Added description, @param, @usage to..
+let _ = function(some, debug, text, to) print(Array.slice(arguments).join(" "));

> >+function run_test() {
> nit: brace on newline please
We're in jsland here! ;) But added

> >+  [["browser/nav-history-service;1", "nsINavHistoryService", 
> nit: I'd rather have a global array of functions that we determine shouldn't
Added _ "comment" describing the fields and moved it global with name.

Comment 14

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/6c48080dba78

For public entry points of nsNavHistory, add ENSURE_ARG for in-pointers and ENSURE_ARG_POINTER for out-pointers; NS_ASSERTION for private APIs. Test automatically finds functions from nav history service and calls them with null arguments.

Comment 15

[Dietrich Ayala (:dietrich)]

Ed, is this port-able to branch? If so, please request approval.

Comment 16

[Ed Lee :Mardak]

Comment on attachment 376812 [details] [diff] [review]
v2.1

(In reply to comment #15)
> Ed, is this port-able to branch? If so, please request approval.
It applied fine with fuzz for 2 spots:

hunk 13: (whitespace)
nsISupports* aUserData)
nsISupports* aUserData) 

hunk 18:
ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver,
ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver_MOZILLA_1_9_1_ADDITIONS,

Comment 17

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

Comment on attachment 376812 [details] [diff] [review]
v2.1

a191=beltzner

but I'm not sure that it's essential to take this; shouldn't we be specifying that we don't accept null params?

Comment 18

[Shawn Wilsher :sdwilsh]

(In reply to comment #17)
> but I'm not sure that it's essential to take this; shouldn't we be specifying
> that we don't accept null params?
Not per xpcom rules sadly.  We aren't supposed to crash at least.

Comment 19

[Ed Lee :Mardak]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/367553b3abf1

Comment 20

[Marco Bonardo [:mak]]

additional changeset due to the fact bug 491983 has been pushed to 1.9.1 before this one, so this patch was missing those changes.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9821af3baba9

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Marking verified based on all tests passes on trunk and 1.9.1.