Closed Bug 524972 Opened 10 years ago Closed 10 years ago

Remove nsTArray from nsINavBookmarksService.idl

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: neil, Assigned: neil)

References

Details

Attachments

(1 file)

nsTArray is not safe across module boundaries, so it should not be used in idl.
is nsIArray safe?
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.
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);|.
Attached patch Proposed patchSplinter Review
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #408868 - Flags: review?(mak77)
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"
Attachment #408868 - Flags: review?(mak77) → review+
(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...
(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 :)
Pushed changeset 17addc6beaed to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
so, why did you add NS_PRECONDITION but did not remove NS_ENSURE_ARG? the first should be enough.
(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.
(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.
we also run test automatically in debug mode... btw, for now that won't hurt.
(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.
All API entries that are supposed to not receive nulls are null checked, we have a test for that.
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?
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
(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.
You need to log in before you can comment on or make changes to this bug.