Closed Bug 788612 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox18 --- fixed

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch patch v.1 (obsolete) — Splinter Review
Assignee: nobody → doug.turner
Attachment #659290 - Flags: review?(bent.mozilla)
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 :)
Attached patch patch v.1Splinter Review
Attachment #659290 - Attachment is obsolete: true
Attachment #659290 - Flags: review?(bent.mozilla)
Attachment #660209 - Flags: review?(bent.mozilla)
Blocks: 786922
QA Contact: jshih
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.
Attachment #660209 - Flags: review?(bent.mozilla) → review+
fixed up.

https://hg.mozilla.org/mozilla-central/rev/b040fefd038e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/7c8b9e22567e
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
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?
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?
John please open a new bug.
You need to log in before you can comment on or make changes to this bug.