783869 - Convert nsDirectoryService::mProviders from nsISupportsArray to nsTArray

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Some lines involving nsDirectoryService::mProviders seem to be causing mysterious build failures in old gcc with nsresult converted to an enum class.  It's probably a compiler bug, and the easiest thing seems to be to work around it: I can just rewrite it to not use nsISupportsArray.  I don't know if that will fix the problem, but cleanup doesn't hurt.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 -- Convert nsDirectoryService::mProviders to nsTArray

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2 -- Make FindProviderFile type-safe

While I'm here . . .

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 3 -- Make nsDirectoryService::RealInit infallible

Cumulative diffstat:

 3 files changed, 25 insertions(+), 59 deletions(-)

Comment 4

[David Zbarsky (:dzbarsky)]

Comment on attachment 653161 [details] [diff] [review]
Patch part 1 -- Convert nsDirectoryService::mProviders to nsTArray

Review of attachment 653161 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/io/nsDirectoryService.cpp
@@ +272,5 @@
>      // Let the list hold the only reference to the provider.
>      nsAppFileLocationProvider *defaultProvider = new nsAppFileLocationProvider;
>      if (!defaultProvider)
>          return NS_ERROR_OUT_OF_MEMORY;
> +    self->mProviders.AppendElement(defaultProvider);

Since we have infallible new, you can just do  | gService = new nsDirectoryService() | above instead of swapping it at the end, and also remove the OOM checks.  After that, this function only returns NS_OK, so perhaps it should be void.

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 4 -- Remove unnecessary swap()

(In reply to David Zbarsky (:dzbarsky) from comment #4)
> Since we have infallible new, you can just do  | gService = new
> nsDirectoryService() | above instead of swapping it at the end

Okay.

> and also
> remove the OOM checks.  After that, this function only returns NS_OK, so
> perhaps it should be void.

These I already did in part 3.

Comment 6

[Benjamin Smedberg]

Comment on attachment 653161 [details] [diff] [review]
Patch part 1 -- Convert nsDirectoryService::mProviders to nsTArray

In nsDirectoryService::RegisterProvider, please add back the !prov null-check. r=me with that change

Comment 7

[Benjamin Smedberg]

Comment on attachment 653718 [details] [diff] [review]
Patch part 4 -- Remove unnecessary swap()

This doesn't look correct, I think you're missing an addref somewhere. Prior to this patch gService should end up with a refcount of 1.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Comment on attachment 653161 [details] [diff] [review]
> Patch part 1 -- Convert nsDirectoryService::mProviders to nsTArray
> 
> In nsDirectoryService::RegisterProvider, please add back the !prov
> null-check. r=me with that change

What do you mean?  I kept the !prov null check, I only removed the !mProviders check.  That no longer makes sense, since mProviders is now an nsTArray and not a pointer to anything.

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> Comment on attachment 653718 [details] [diff] [review]
> Patch part 4 -- Remove unnecessary swap()
> 
> This doesn't look correct, I think you're missing an addref somewhere. Prior
> to this patch gService should end up with a refcount of 1.

Hmm . . . would it be okay to make gService an nsRefPtr, or would that be a problem because we want to avoid static initializers?  Can we rely on compilers to not actually run any code at startup to initialize a static nsRefPtr to nullptr?  If we can't make gService an nsRefPtr for whatever reason, I could just add NS_IF_RELEASE() and NS_ADDREF().

CCing Taras for the "is static nsRefPtr member bad" question.

Comment 10

[Benjamin Smedberg]

re: !prov I totally misread the diff, sorry!

You want StaticRefPtr, see xpcom/base/StaticPtr.h and bug 772987.

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I have a new patch part 4, but two consecutive try runs based on different base revisions gave the same very puzzling failures for the patch series:

https://tbpl.mozilla.org/?tree=Try&rev=92d82878f530
https://tbpl.mozilla.org/?tree=Try&rev=637ee096a896

Any ideas on what might be causing these failures?

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Might be related to bug 785102 and bug 785828.

Comment 13

[Benjamin Smedberg]

I doubt it. Aryeh, probably the easiest way to reproduce that is by running `make package` in your local build. Perhaps just launching xpcshell would reproduce? My spider-sense says this is a refcounting error (double-free), but a super-quick patch scan didn't find it.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 4, v2 -- Remove unnecessary swap()

Just posting this in case anyone else ever wants to pick it up.  It continues to cause weird errors, including locally, and I don't think it's worth the time to track down.  I'll just push the first three parts.

Try for first three parts: https://tbpl.mozilla.org/?tree=Try&rev=308e5f61e661

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The first patch here causes a new failure when running "make check" in xpcom/tests/.  Excerpt:

<<<<
END unit tests for |nsRefPtr|.
WARNING: NS_ENSURE_SUCCESS(rv, nullptr) failed with result 0x80004005: file /mnt/ssd/checkouts/central/dom/indexedDB/IndexedDatabaseManager.cpp, line 226
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80570016: file /mnt/ssd/checkouts/central/xpcom/tests/TestSettingsAPI.cpp, line 224
WARNING: NS_ENSURE_SUCCESS(rv, 1) failed with result 0x80570016: file /mnt/ssd/checkouts/central/xpcom/tests/TestSettingsAPI.cpp, line 292
WARNING: nsExceptionService ignoring thread destruction after shutdown: file /mnt/ssd/checkouts/central/xpcom/base/nsExceptionService.cpp, line 166
TEST-PASS | Start TestSettingsAPI
Running TestSettingsAPI tests...
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: file:///mnt/ssd/checkouts/central/objdir-ff-release/dist/bin/components/SettingsService.js :: SettingsService :: line 153"  data: no]
************************************************************
Finished running TestSettingsAPI tests.
>>>>

The exception on line 224 of TestSettingsAPI.cpp is caused by:

  nsCOMPtr<nsISettingsService> settingsService =
    do_CreateInstance("@mozilla.org/settingsService;1", &rv);
  NS_ENSURE_SUCCESS(rv, rv);

Any idea what that signifies?

Comment 16

[Josh Matthews [:jdm]]

It means that an error was thrown while obtaining the new instance of the settings service. Given the code at http://mxr.mozilla.org/mozilla-central/source/dom/settings/SettingsService.js#148, my money is on http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IndexedDatabaseManager.cpp#1844.

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Okay, I couldn't figure out how to run a single xpcshell test in a debugger, but it turned out just as well -- code inspection of the patch turned up the bug more easily anyway.  Interdiff of patch part 1 that causes the test to pass locally:

diff --git a/xpcom/io/nsDirectoryService.cpp b/xpcom/io/nsDirectoryService.cpp
--- a/xpcom/io/nsDirectoryService.cpp
+++ b/xpcom/io/nsDirectoryService.cpp
@@ -393,17 +393,19 @@ nsDirectoryService::Get(const char* prop
         cachedFile->Clone(getter_AddRefs(cloneFile));
         return cloneFile->QueryInterface(uuid, result);
     }
 
     // it is not one of our defaults, lets check any providers
     FileData fileData(prop, uuid);
 
     for (int32_t i = mProviders.Length() - 1; i >= 0; i--) {
-        FindProviderFile(mProviders[i], &fileData);
+        if (!FindProviderFile(mProviders[i], &fileData)) {
+            break;
+        }
     }
     if (fileData.data)
     {
         if (fileData.persistent)
         {
             Set(prop, static_cast<nsIFile*>(fileData.data));
         }
         nsresult rv = (fileData.data)->QueryInterface(uuid, result);

I wasn't properly emulating the behavior of nsSupportsArray::EnumerateBackwards.  New try: https://tbpl.mozilla.org/?tree=Try&rev=b24ec96162e3

Will re-request review once this passes try.

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1, v2 -- Convert nsDirectoryService::mProviders to nsTArray

Now passes try.  See previous comment for interdiff and explanation -- EnumerateBackwards() doesn't just enumerate backwards, it bails out if the function returns false.  Why this was viewed as useful when a three-line loop does the same thing much more clearly and flexibly is beyond me.

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f17fe6cdea2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/54a5072fe775
https://hg.mozilla.org/integration/mozilla-inbound/rev/5642666e6f9e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f17fe6cdea2a
https://hg.mozilla.org/mozilla-central/rev/54a5072fe775
https://hg.mozilla.org/mozilla-central/rev/5642666e6f9e