524972 - Remove nsTArray from nsINavBookmarksService.idl

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[[e-mail]]

nsTArray is not safe across module boundaries, so it should not be used in idl.

Comment 1

[Marco Bonardo [:mak]]

is nsIArray safe?

Comment 2

[[e-mail]]

It's horrible to use for PRInt64. [array, retval, size_is(count)] out long long is actually reasonably easy to use from C++, although Places should of course continue to use nsTArray internally.

Comment 3

[Shawn Wilsher :sdwilsh]

We have
527   void getBookmarkIdsForURI(in nsIURI aURI, out unsigned long count,
528                             [array, retval, size_is(count)] out long long bookmarks);
For JS, so this is only a noscript method change.  We should really just get rid of the noscript method, and use this.  We should also make count [optional] so script can just call |bs.getBookmarkIdsForURI(uri);|.

Comment 4

[[e-mail]]

Attachment: Proposed patch

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 408868 [details] [diff] [review]
Proposed patch

I'm fine with removing that API as well.

>-NS_IMETHODIMP
>+nsresult
> nsNavBookmarks::GetBookmarkIdsForURITArray(nsIURI *aURI,
>-                                         nsTArray<PRInt64> *aResult) 
>+                                           nsTArray<PRInt64> &aResult) 
> {
>   NS_ENSURE_ARG(aURI);

since this is no more exposed in API we can probably assert here, it's internal code.

>   mozStorageStatementScoper scope(mDBFindURIBookmarks);
> 
>@@ -2708,7 +2707,7 @@
> 
>   PRBool more;
>   while (NS_SUCCEEDED((rv = mDBFindURIBookmarks->ExecuteStep(&more))) && more) {
>-    if (! aResult->AppendElement(
>+    if (! aResult.AppendElement(

while here please remove that space after the "!"

>diff -r 22010cbc1269 toolkit/components/places/src/nsNavBookmarks.h
>--- a/toolkit/components/places/src/nsNavBookmarks.h	Tue Oct 27 13:54:54 2009 -0700
>+++ b/toolkit/components/places/src/nsNavBookmarks.h	Wed Oct 28 15:33:20 2009 +0000
>@@ -259,6 +259,14 @@
>                               const nsAString &aServiceContractId,
>                               PRInt64 *_retval);
> 
>+  /**
>+   * TArray version of getBookmarksIdForURI for ease of use in C++ code.
>+   * Pass in a reference to a TArray; it will get filled with the
>+   * resulting list of folder IDs.
>+   */

could you make this a proper javadoc with @param?

Also "list of folder IDs" is wrong should be "list of bookmarks IDs"

Comment 6

[[e-mail]]

(In reply to comment #5)
>>   NS_ENSURE_ARG(aURI);
>since this is no more exposed in API we can probably assert here, it's internal
>code.
I'm not convinced that all of the callers check.

>>-    if (! aResult->AppendElement(
>>+    if (! aResult.AppendElement(
> while here please remove that space after the "!"
Sure.

> >+  /**
> >+   * TArray version of getBookmarksIdForURI for ease of use in C++ code.
> >+   * Pass in a reference to a TArray; it will get filled with the
> >+   * resulting list of folder IDs.
> >+   */
> could you make this a proper javadoc with @param?
Not until someone makes getBookmarksIdForURI a proper javadoc with @param...

> Also "list of folder IDs" is wrong should be "list of bookmarks IDs"
Bah, I already fixed one documentation error, but I missed that...

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)
> (In reply to comment #5)
> >>   NS_ENSURE_ARG(aURI);
> >since this is no more exposed in API we can probably assert here, it's internal
> >code.
> I'm not convinced that all of the callers check.

then we should catch it in debug builds :)

Comment 8

[[e-mail]]

Pushed changeset 17addc6beaed to mozilla-central.

Comment 9

[Marco Bonardo [:mak]]

so, why did you add NS_PRECONDITION but did not remove NS_ENSURE_ARG? the first should be enough.

Comment 10

[[e-mail]]

(In reply to comment #9)
> so, why did you add NS_PRECONDITION but did not remove NS_ENSURE_ARG? the first
> should be enough.
I'm still not convinced all the callers check.

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #10)
> (In reply to comment #9)
> > so, why did you add NS_PRECONDITION but did not remove NS_ENSURE_ARG? the first
> > should be enough.
> I'm still not convinced all the callers check.

the callers are just us (Places), and since all of us use debug mode, i don't see the problem.

Comment 12

[Marco Bonardo [:mak]]

we also run test automatically in debug mode... btw, for now that won't hurt.

Comment 13

[[e-mail]]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > so, why did you add NS_PRECONDITION but did not remove NS_ENSURE_ARG? the first
> > > should be enough.
> > I'm still not convinced all the callers check.
> the callers are just us (Places), and since all of us use debug mode, i don't
> see the problem.
Nightly builds won't have debug mode, so if some badly-written extension passes in a null URI to one of the callers then you'll crash.

Comment 14

[Marco Bonardo [:mak]]

All API entries that are supposed to not receive nulls are null checked, we have a test for that.

Comment 15

[[e-mail]]

Sure, they do now, but you're asking me to remove a null check - who's going to fix things so that all the tests pass again?

Comment 16

[Shawn Wilsher :sdwilsh]

It's a redundant null check.  All public API points do proper null checking and are tested in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/unit/test_null_interfaces.js

Comment 17

[[e-mail]]

(In reply to comment #16)
> It's a redundant null check.
It didn't look safe on first glance, but because it's you, I double-checked.

Some of them directly check their aURI. Some of them create their own URI. I already knew these were safe.
Some of them pass aURI to IsBookmarked, which checks the aURI, and since they initialise the outparam to false, this does in fact work.
And OnPageChanged calls a method on aURI without null-checking it, so I indirectly know that it's not null before GetBookmarkIdsForURITArray sees it.