788612 - Device Storage - Move bundle handing into nsDOMDeviceStorage to avoid calls to the nsIStringBundleService.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Doug Turner (:dougt)]

We have doubts that the nsIStringBundleService is safe to call from multiple threads.  We also think it would be faster to cache the strings we need and void the nsIStringBundleService all together in the IsType() checks.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 659290 [details] [diff] [review]
patch v.1

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

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +59,5 @@
> +DeviceStorageTypeChecker sDeviceStorageTypeChecker;
> +
> +static const PRUnichar* const kDeviceStoragePictures = NS_LITERAL_STRING("pictures").get();
> +static const PRUnichar* const kDeviceStorageVideos = NS_LITERAL_STRING("videos").get();
> +static const PRUnichar* const kDeviceStorageMusic = NS_LITERAL_STRING("music").get();

This isn't safe... The pointer returned by get() is only valid so long as the object that created it exists, and these temporaries are dying almost as soon as they're created. (Since it's a dependent string I think this is only really a problem for gcc < 4.4 and other compilers without char16_t or short wchar_t, but so far as I know we can't assume such support).

Your choices are either:

  static const PRUnichar kDeviceStoragePictures[] = { 'p', 'i', ..., '\0' };

or:

  #define PICTURES_STRING "pictures"
  ...
  if (aType.EqualsLiteral(PICTURES_STRING)) {

@@ +72,5 @@
> +
> +DeviceStorageTypeChecker::~DeviceStorageTypeChecker()
> +{
> +  if (mPicturesExtensions) {
> +    nsMemory::Free(mPicturesExtensions);

Here you're using xpcom function in a static destructor, and xpcom could have already been unloaded by the time this static destructor runs which will crash. It depends on how we link, XULRunner apps in particular used to get killed by stuff like this.

You could rework this to take advantage of ClearOnShutdown and then it would be safe.

@@ +115,5 @@
> +{
> +  NS_ASSERTION(aBlob, "Calling Check without a blob");
> +
> +  if (!mInited) {
> +    return false;

This is potentially racy... There must be higher logic that ensures mInited must be true before another thread can access this, right? If so then it would make more sense to assert mInited.

@@ +156,5 @@
> +    return false;
> +  }
> +
> +  nsAutoString extensionMatch;
> +  extensionMatch.AssignASCII("*");

AppendLiteral, here and below, avoids strlen calls.

@@ +162,5 @@
> +  extensionMatch.AppendASCII(";");
> +
> +  if (mPicturesExtensions && aType.Equals(kDeviceStoragePictures)) {
> +    return FindInReadable(extensionMatch, nsDependentString(mPicturesExtensions));
> +  } 

Nit: trailing whitespace

::: dom/devicestorage/nsDeviceStorage.h
@@ +40,5 @@
>  using namespace mozilla::dom;
>  
> +class DeviceStorageTypeChecker MOZ_FINAL
> +{
> + public:

Nit: your other helper classes don't have any indent here. I don't care which you choose but be consistent.

@@ +50,5 @@
> +  bool Check(const nsAString& aType, nsIDOMBlob* aBlob);
> +  bool Check(const nsAString& aType, nsIFile* aFile);
> +
> + private:
> +  bool mInited;

Nit: It's generally better to put bools at the end of the member list so that they might pack better. Here it won't make any difference so whatever :)

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 660209 [details] [diff] [review]
patch v.1

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

Looks good!

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +83,5 @@
> +  nsCOMPtr<nsIStringBundleService> stringService = mozilla::services::GetStringBundleService();
> +  if (!stringService) {
> +    return nullptr;
> +  }
> +  

Nit: trailing whitespace.

::: dom/devicestorage/nsDeviceStorage.h
@@ +59,5 @@
> +  nsString mPicturesExtensions;
> +  nsString mVideosExtensions;
> +  nsString mMusicExtensions;
> +
> +  static nsAutoPtr<DeviceStorageTypeChecker> gDeviceStorageTypeChecker;

Nit: 's' is the normal prefix for static members.

Comment 5

[Doug Turner (:dougt)]

fixed up.

https://hg.mozilla.org/mozilla-central/rev/b040fefd038e

Comment 6

[Ed Morley [:emorley]]

Push backed out for introducing new M2 orange in test_diskSpace.html & backout of the cset that landed it having conflicts. 

Also, please do not push to mozilla-central unless you are going to watch/star your push (sheriffs are going to be getting more strict about tree rule violations; ie backout).

https://hg.mozilla.org/mozilla-central/rev/5faccd0b7618

Comment 7

[Doug Turner (:dougt)]

https://hg.mozilla.org/mozilla-central/rev/7c8b9e22567e

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 660209 [details] [diff] [review]
patch v.1

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

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +60,5 @@
>  using namespace mozilla::dom::devicestorage;
>  
>  #include "nsDirectoryServiceDefs.h"
>  
> +nsAutoPtr<DeviceStorageTypeChecker> DeviceStorageTypeChecker::gDeviceStorageTypeChecker;

Not StaticAutoPtr?

Comment 9

[John Shih]

Hi Doug, 
when I verify this bug, i met this situation:
there are five music files in /sdcard/Music, 
four default ogg file(Salt_Creek.ogg, b2g.ogg, jonobacon_island_01-02_stevenson.ogg, treasure_island_01-02_stevenson.ogg)
and one my own mp3 file
I can see&play them all in the Playlist > All Songs
But in Mix page, I only can see three of them(b2g.ogg, jonobacon_island_01-02_stevenson.ogg, treasure_island_01-02_stevenson.ogg)
And I only can play jonobacon_island_01-02_stevenson.ogg by press the block on mix page
while the other two can't
there is the logcat for when I press the block of the other two file(b2g.ogg, jonobacon_island_01-02_stevenson.ogg) in attachment 

Can you check it's a gecko bug or gaia bug?

Comment 10

[John Shih]

Attachment: Press the block of music file in mix page and it won't be played

Comment 11

[Doug Turner (:dougt)]

John please open a new bug.