See bug 989020.

This patch adds a Main Thread listener for the preferences we're interested in, and when they change, updates local cached versions that are held behind a read-write mutex.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=834fd4fddee0

And that, as they say, is why we have try servers: https://tbpl.mozilla.org/?tree=Try&rev=0661a4f12ddb

Pref changes are listened-for on the Main Thread, and values cached behind a RWLock for use off-Main Thread. In case it's a concern, tThe extra initialization in nsLayoutStatics::Initialize() takes ~4.6ms running in the ARM emulator on my Thinkpad X220, so should not be a concern on real hardware.

Ben, When you get a chance, please review Mike's patch. Thanks Hema

Hey, sorry, will get to this soon. It got backed up behind several blockers and other large reviews.

Hrm, looking at this patch it seems like this is a bit more complicated than I expected it to be. I can give you some guidance here to simplify it a bunch I think. The main question is why these prefs need the perf advantage of RW locks (assuming our crazy old rw locks are actually implemented well...). It looks to me like these prefs are only being used during testing, so I don't see the benefit of using RW locks really.

bent, the RW locks are used pretty extensively in the CameraParameters code, and seem to work as expected. :) I'm open to any advice you've got.

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #7) Ok, but the main question here is why you need the RW locks for these testing-only prefs?

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #9) > > Ok, but the main question here is why you need the RW locks for these > testing-only prefs? Sorry--first day back, sorting through 6000+ emails. :) The RW locks protect the cached pref values which are accessed on and off the Main Thread. Any other sort of mutex would work--I just used these since there a definite separation between readers and writers.

Comment on attachment 8416655 [details] [diff] [review] Handle Camera preferences on Main Thread, v2.2 Review of attachment 8416655 [details] [diff] [review]: ----------------------------------------------------------------- Ok, so rather than use explicit readwrite locks, static initialization, PR_CallOnce, etc. I think it would be much simpler to just use atomic values (see mfbt/Atomics.h). You will still need a pref callback to set them but you won't need to worry about locks whatsoever. ::: dom/camera/CameraPreferences.h @@ +7,5 @@ > +#define DOM_CAMERA_CAMERAPREFERENCES_H > + > +#include "nsString.h" > + > +namespace mozilla { You're missing the dom namespace too. ::: dom/camera/moz.build @@ +8,5 @@ > TEST_DIRS += ['test'] > > EXPORTS += [ > 'CameraCommon.h', > + 'CameraPreferences.h', It doesn't look like this really needs to be exported, right? You only ever seem to use it in this one directory.

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #11) > Comment on attachment 8416655 [details] [diff] [review] > Handle Camera preferences on Main Thread, v2.2 > > Review of attachment 8416655 [details] [diff] [review]: > ----------------------------------------------------------------- > > Ok, so rather than use explicit readwrite locks, static initialization, > PR_CallOnce, etc. I think it would be much simpler to just use atomic values > (see mfbt/Atomics.h). You will still need a pref callback to set them but > you won't need to worry about locks whatsoever. Looking at mfbt/Atomics.h, I don't see support for anything like Atomic<nsCString> (and a dxr search shows no atomic string types either). I could use Atomic<const char*> but that seems more error-prone. > ::: dom/camera/CameraPreferences.h > @@ +7,5 @@ > > +#define DOM_CAMERA_CAMERAPREFERENCES_H > > + > > +#include "nsString.h" > > + > > +namespace mozilla { > > You're missing the dom namespace too. None of the other files in dom/camera use the dom namespace either. Should I add it to all of them? > ::: dom/camera/moz.build > @@ +8,5 @@ > > TEST_DIRS += ['test'] > > > > EXPORTS += [ > > 'CameraCommon.h', > > + 'CameraPreferences.h', > > It doesn't look like this really needs to be exported, right? You only ever > seem to use it in this one directory. The initializer which registers for the prefs is called from layout/build/nsLayoutStatics.cpp.

Need follow-up to comment 12.

Ah, I missed the string pref somehow, sorry! I guess that means you need a lock after all, but I'd just use a regular one rather than the readwrite ones.

Incorporate feedback from earlier review. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=182a38b2d58f

Hmm, looks like TBPL doesn't like the static ReentrantMonitor: 13:50:50 INFO - TEST-INFO | leakcheck | leaked 1 ReentrantMonitor (32 bytes) 13:50:50 WARNING - TEST-UNEXPECTED-FAIL | leakcheck | 32 bytes leaked (ReentrantMonitor)

bent: review ping? (Or if you're swamped, HTTP 301?)

Comment on attachment 8431802 [details] [diff] [review] Handle Camera preferences on Main Thread, v3 Review of attachment 8431802 [details] [diff] [review]: ----------------------------------------------------------------- Minusing mostly because of the two big issues mentioned in CameraPreferences.h. ::: dom/camera/CameraPreferences.cpp @@ +11,5 @@ > + > +using namespace mozilla; > + > +ReentrantMonitor > + CameraPreferences::sPrefMonitor("CameraPreferences.sPrefMonitor"); I think adding global monitors like this will result in static constructors. Please double check with froydnj? @@ +103,5 @@ > +{ > + DOM_CAMERA_LOGI("Preference '%s' has changed\n", aPref); > + ReentrantMonitorAutoEnter mon(sPrefMonitor); > + > + for (uint32_t i = 0; i < sizeof(sPrefs) / sizeof(sPrefs[0]); ++i) { Nit: please use mozilla::ArrayLength. @@ +120,5 @@ > +/* static */ > +bool > +CameraPreferences::Initialize() > +{ > + ReentrantMonitorAutoEnter mon(sPrefMonitor); I think the caller needs to hold the monitor. @@ +126,5 @@ > + if (sPrefsRegistered) { > + return true; > + } > + if (!NS_IsMainThread()) { > + NS_WARNING("Can't initialize CameraPreferences off Main Thread\n"); I think you should MOZ_ASSERT this instead. This code should never really be called from non-main-thread. ::: dom/camera/CameraPreferences.h @@ +28,5 @@ > + static bool HasCameraControlMethodErrorOverride(); > + static nsresult GetCameraControlMethodErrorOverride(); > + > + static bool HasCameraControlAsyncErrorOverride(); > + static nsresult GetCameraControlAsyncErrorOverride(); Hmm, there is a *ton* of repeated boiler place code. Can we design something simpler here? Like: nsAdoptingCString GetCString(const char* aPrefName); nsresult GetNSResult(const char* aPrefName); It seems like we'd be able to remove a lot of this boiler plate if we just keep using the string name of the prefs, which is more in line with the existing Preferences API. @@ +54,5 @@ > + UpdatePref mUpdate; > + }; > + static Pref sPrefs[]; > + > + static ReentrantMonitor sPrefMonitor; Why do you need a reentrant monitor here?

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #18) > Comment on attachment 8431802 [details] [diff] [review] > Handle Camera preferences on Main Thread, v3 > > Review of attachment 8431802 [details] [diff] [review]: > ----------------------------------------------------------------- > > Minusing mostly because of the two big issues mentioned in > CameraPreferences.h. > > ::: dom/camera/CameraPreferences.cpp > @@ +11,5 @@ > > + > > +using namespace mozilla; > > + > > +ReentrantMonitor > > + CameraPreferences::sPrefMonitor("CameraPreferences.sPrefMonitor"); > > I think adding global monitors like this will result in static constructors. > Please double check with froydnj? I spoke to froydnj and he says you're right; he also suggested a different pattern to implement this properly. > @@ +120,5 @@ > > +/* static */ > > +bool > > +CameraPreferences::Initialize() > > +{ > > + ReentrantMonitorAutoEnter mon(sPrefMonitor); > > I think the caller needs to hold the monitor. I don't see why. Protecting 'sPrefsRegistered' should be fine, no? > @@ +126,5 @@ > > + if (sPrefsRegistered) { > > + return true; > > + } > > + if (!NS_IsMainThread()) { > > + NS_WARNING("Can't initialize CameraPreferences off Main Thread\n"); > > I think you should MOZ_ASSERT this instead. This code should never really > be called from non-main-thread. Okay. > ::: dom/camera/CameraPreferences.h > @@ +28,5 @@ > > + static bool HasCameraControlMethodErrorOverride(); > > + static nsresult GetCameraControlMethodErrorOverride(); > > + > > + static bool HasCameraControlAsyncErrorOverride(); > > + static nsresult GetCameraControlAsyncErrorOverride(); > > Hmm, there is a *ton* of repeated boiler place code. Can we design > something simpler here? Like: > > nsAdoptingCString GetCString(const char* aPrefName); > nsresult GetNSResult(const char* aPrefName); > > It seems like we'd be able to remove a lot of this boiler plate if we just > keep using the string name of the prefs, which is more in line with the > existing Preferences API. I tried to keep the boilerplate is pretty simple. The problem is in order to register for Main Thread preference events, I need to know the values to listen for beforehand; those are the values in the 'sPrefs' array. Since I already know those values, I decided to expose them as separately named methods, instead of having to match a pref key one every access. > @@ +54,5 @@ > > + UpdatePref mUpdate; > > + }; > > + static Pref sPrefs[]; > > + > > + static ReentrantMonitor sPrefMonitor; > > Why do you need a reentrant monitor here? To provide mutual exclusion to the cached preference values.

To clarify that last comment: to provide mutual exclusion to the cached preference values, because the cached values are updated on the Main Thread, but may be accessed from workers.

(In reply to Mike Habicher [:mikeh] from comment #19) > > @@ +120,5 @@ > > > +/* static */ > > > +bool > > > +CameraPreferences::Initialize() > > > +{ > > > + ReentrantMonitorAutoEnter mon(sPrefMonitor); > > > > I think the caller needs to hold the monitor. > > I don't see why. Protecting 'sPrefsRegistered' should be fine, no? Actually on a second look, CameraPreferences::Initialize() is called from nsLayoutStatics::Initialize() and as far as I can see that is the only place where we set sPrefsRegistered. What are you trying to protect against with this lock? > > ::: dom/camera/CameraPreferences.h > > @@ +28,5 @@ > > > + static bool HasCameraControlMethodErrorOverride(); > > > + static nsresult GetCameraControlMethodErrorOverride(); > > > + > > > + static bool HasCameraControlAsyncErrorOverride(); > > > + static nsresult GetCameraControlAsyncErrorOverride(); > > > > Hmm, there is a *ton* of repeated boiler place code. Can we design > > something simpler here? Like: > > > > nsAdoptingCString GetCString(const char* aPrefName); > > nsresult GetNSResult(const char* aPrefName); > > > > It seems like we'd be able to remove a lot of this boiler plate if we just > > keep using the string name of the prefs, which is more in line with the > > existing Preferences API. > > I tried to keep the boilerplate is pretty simple. Yep, but I'm suggesting that we should make it even simpler. :-) > The problem is in order to register for Main Thread preference events, I > need to know the values to listen for beforehand; those are the values in > the 'sPrefs' array. Since I already know those values, I decided to expose > them as separately named methods, instead of having to match a pref key one > every access. Yeah, I'm not suggesting changing how sPrefs works, I just don't understand why we need to have one method per preference like this. That seems like needless compexity, and it also makes this code not follow the existing Preferences conventions which is suboptimal. > > @@ +54,5 @@ > > > + UpdatePref mUpdate; > > > + }; > > > + static Pref sPrefs[]; > > > + > > > + static ReentrantMonitor sPrefMonitor; > > > > Why do you need a reentrant monitor here? > > To provide mutual exclusion to the cached preference values. My question was mostly about the reentrant part. I agree that we need to protect against concurrent access to the cached values.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #21) > > Actually on a second look, CameraPreferences::Initialize() is called from > nsLayoutStatics::Initialize() and as far as I can see that is the only place > where we set sPrefsRegistered. What are you trying to protect against with > this lock? Good point. This code has changed significantly over its review lifetime. I'll remove this. > Yeah, I'm not suggesting changing how sPrefs works, I just don't understand > why we need to have one method per preference like this. That seems like > needless compexity, and it also makes this code not follow the existing > Preferences conventions which is suboptimal. I originally thought about making this a more general and extensible pref wrapper, but decided that would be more complex: matching the pref key string, making sure the pref value matches the getter type. I decided just having named getters was much simpler, and lets the compiler do the heavy lifting. > My question was mostly about the reentrant part. I agree that we need to > protect against concurrent access to the cached values. Oh, I see what you mean. I'll change it to a basic Monitor. Thanks!

(In reply to Mike Habicher [:mikeh] from comment #22) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #21) > > > > Actually on a second look, CameraPreferences::Initialize() is called from > > nsLayoutStatics::Initialize() and as far as I can see that is the only place > > where we set sPrefsRegistered. What are you trying to protect against with > > this lock? > > Good point. This code has changed significantly over its review lifetime. > I'll remove this. Thanks! > > Yeah, I'm not suggesting changing how sPrefs works, I just don't understand > > why we need to have one method per preference like this. That seems like > > needless compexity, and it also makes this code not follow the existing > > Preferences conventions which is suboptimal. > > I originally thought about making this a more general and extensible pref > wrapper, but decided that would be more complex: matching the pref key > string, making sure the pref value matches the getter type. I decided just > having named getters was much simpler, and lets the compiler do the heavy > lifting. OK, fine. I still don't understand the complexity concern that you mention but if you prefer to have two functions per every preference I guess I won't hold the patch. :/ > > My question was mostly about the reentrant part. I agree that we need to > > protect against concurrent access to the cached values. > > Oh, I see what you mean. I'll change it to a basic Monitor. Thanks! Cool, thanks.

Incorporate review feedback. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=1b671f332aef

Comment on attachment 8441376 [details] [diff] [review] Handle Camera preferences on Main Thread, v4 Review of attachment 8441376 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/camera/CameraPreferences.cpp @@ +65,5 @@ > +/* static */ > +nsresult > +CameraPreferences::UpdateCameraControlMethodError(const char* aPref) > +{ > + return UpdateNsResult(aPref, sPrefCameraControlMethodErrorOverride); Shouldn't these methods hold a lock? @@ +199,5 @@ > +// static > +nsCString > +CameraPreferences::GetEnabledTest() > +{ > + return GetPreferenceValue(sPrefTestEnabled); This seems a bit fishy since you grab a reference to the variables before holding the lock and then hold the lock before you read the members, so you'd really be relying on how the compiler optimizes this code. The correct way to do this would be to hold the lock in these methods and then access the member variable. Which I think eliminates the value of these GetPreferences/HasPreferences helpers, which I mentioned earlier.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #25) > Comment on attachment 8441376 [details] [diff] [review] > Handle Camera preferences on Main Thread, v4 > > Review of attachment 8441376 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/camera/CameraPreferences.cpp > @@ +65,5 @@ > > +/* static */ > > +nsresult > > +CameraPreferences::UpdateCameraControlMethodError(const char* aPref) > > +{ > > + return UpdateNsResult(aPref, sPrefCameraControlMethodErrorOverride); > > Shouldn't these methods hold a lock? The pref-changed callbacks go through the PreferenceChanged() member (see line 110) which holds the lock. > @@ +199,5 @@ > > +// static > > +nsCString > > +CameraPreferences::GetEnabledTest() > > +{ > > + return GetPreferenceValue(sPrefTestEnabled); > > This seems a bit fishy since you grab a reference to the variables before > holding the lock and then hold the lock before you read the members, so > you'd really be relying on how the compiler optimizes this code. The > correct way to do this would be to hold the lock in these methods and then > access the member variable. Which I think eliminates the value of these > GetPreferences/HasPreferences helpers, which I mentioned earlier. Yes, that's definitely a problem. Okay, I'll rework this.

(In reply to Mike Habicher [:mikeh] from comment #26) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #25) > > Comment on attachment 8441376 [details] [diff] [review] > > Handle Camera preferences on Main Thread, v4 > > > > Review of attachment 8441376 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/camera/CameraPreferences.cpp > > @@ +65,5 @@ > > > +/* static */ > > > +nsresult > > > +CameraPreferences::UpdateCameraControlMethodError(const char* aPref) > > > +{ > > > + return UpdateNsResult(aPref, sPrefCameraControlMethodErrorOverride); > > > > Shouldn't these methods hold a lock? > > The pref-changed callbacks go through the PreferenceChanged() member (see > line 110) which holds the lock. Ah, right. That's super confusing. :( I'd appreciate if you would consider simplifying this code, this patch confuses me every time I read it. > > @@ +199,5 @@ > > > +// static > > > +nsCString > > > +CameraPreferences::GetEnabledTest() > > > +{ > > > + return GetPreferenceValue(sPrefTestEnabled); > > > > This seems a bit fishy since you grab a reference to the variables before > > holding the lock and then hold the lock before you read the members, so > > you'd really be relying on how the compiler optimizes this code. The > > correct way to do this would be to hold the lock in these methods and then > > access the member variable. Which I think eliminates the value of these > > GetPreferences/HasPreferences helpers, which I mentioned earlier. > > Yes, that's definitely a problem. Okay, I'll rework this. Thanks. FWIW what I suggest you should do is either comment 21 or at least try to keep the code that holds locks and the code that uses the variables protected by the locks together. I'm very worried that the current architecture of the patch will make others introduce race conditions in the future by changing part of how this works without understanding the locking implications.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=c21eaf5e4b23

Fix static assertion failures on some DEBUG builds. Incremental try-server push: https://tbpl.mozilla.org/?tree=Try&rev=5e9c2fd4832a

Comment on attachment 8441628 [details] [diff] [review] Handle Camera preferences on Main Thread, v5.1 Review of attachment 8441628 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good! r=me with the comments below fixed. ::: dom/camera/CameraPreferences.cpp @@ +13,5 @@ > +/* statics */ > +bool CameraPreferences::sPrefsRegistered = false; > +nsCString CameraPreferences::sPrefTestEnabled; > +nsCString CameraPreferences::sPrefHardwareTest; > +nsCString CameraPreferences::sPrefGonkParameters; Please check to make sure that these don't add static initializers. @@ +28,5 @@ > + > +/* static */ > +nsresult > +CameraPreferences::UpdatePref(const char* aPref, nsresult& aVal) > + Nit: whitespace. @@ +132,5 @@ > + default: > + MOZ_ASSERT_UNREACHABLE("Unhandled preference value type!"); > + return; > + } > + Nit: whitespace. @@ +133,5 @@ > + MOZ_ASSERT_UNREACHABLE("Unhandled preference value type!"); > + return; > + } > + > + if (NS_FAILED(rv)) { Please surround this condition with #ifdef DEBUG. @@ +146,5 @@ > +CameraPreferences::Initialize() > +{ > + if (sPrefsRegistered) { > + return true; > + } This function should only be called once, so you can remove sPrefsRegistered altogether. @@ +150,5 @@ > + } > + if (!NS_IsMainThread()) { > + MOZ_ASSERT(false, "Can't initialize CameraPreferences off Main Thread\n"); > + return false; > + } The nsLayoutStatics stuff is guaranteed to happen on the main thread, so this can go too. @@ +155,5 @@ > + > + nsresult rv; > + for (uint32_t i = 0; i < ArrayLength(sPrefs); ++i) { > + rv = Preferences::RegisterCallbackAndCall(CameraPreferences::PreferenceChanged, > + sPrefs[i].mPref); I guess this is going to mean that we'd hold the lock in PreferenceChanged needlessly... I can't think of a good way to fix that though. :/ @@ +172,5 @@ > +{ > + MonitorAutoLock mon(PrefMonitor()); > + > + int32_t i = PrefToIndex(aPref); > + if (i < 0 || static_cast<uint32_t>(i) >= ArrayLength(sPrefs)) { You could remove this cast if you made PrefToIndex return a uint32_t and return 0xffffffff in case it can't find the pref or some such. @@ +173,5 @@ > + MonitorAutoLock mon(PrefMonitor()); > + > + int32_t i = PrefToIndex(aPref); > + if (i < 0 || static_cast<uint32_t>(i) >= ArrayLength(sPrefs)) { > + DOM_CAMERA_LOGW("Preference '%s' is not tracked by CameraPreferences\n", aPref); I assume these log macros are safe to call from any thread. ::: dom/camera/CameraPreferences.h @@ +6,5 @@ > +#ifndef DOM_CAMERA_CAMERAPREFERENCES_H > +#define DOM_CAMERA_CAMERAPREFERENCES_H > + > +#include "nsString.h" > +#include "mozilla/Monitor.h" Please move this include to the .cpp file and just forward declare Monitor here.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #30) > Comment on attachment 8441628 [details] [diff] [review] > Handle Camera preferences on Main Thread, v5.1 > > Review of attachment 8441628 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks, looks good! r=me with the comments below fixed. > > ::: dom/camera/CameraPreferences.cpp > @@ +13,5 @@ > > +/* statics */ > > +bool CameraPreferences::sPrefsRegistered = false; > > +nsCString CameraPreferences::sPrefTestEnabled; > > +nsCString CameraPreferences::sPrefHardwareTest; > > +nsCString CameraPreferences::sPrefGonkParameters; > > Please check to make sure that these don't add static initializers. Spoke to froydnj and he said they will. I'll rework this. > @@ +155,5 @@ > > + > > + nsresult rv; > > + for (uint32_t i = 0; i < ArrayLength(sPrefs); ++i) { > > + rv = Preferences::RegisterCallbackAndCall(CameraPreferences::PreferenceChanged, > > + sPrefs[i].mPref); > > I guess this is going to mean that we'd hold the lock in PreferenceChanged > needlessly... I can't think of a good way to fix that though. :/ Maybe some kind of out-of-band flag. X-p > @@ +172,5 @@ > > +{ > > + MonitorAutoLock mon(PrefMonitor()); > > + > > + int32_t i = PrefToIndex(aPref); > > + if (i < 0 || static_cast<uint32_t>(i) >= ArrayLength(sPrefs)) { > > You could remove this cast if you made PrefToIndex return a uint32_t and > return 0xffffffff in case it can't find the pref or some such. Thanks for the suggestion. > @@ +173,5 @@ > > + MonitorAutoLock mon(PrefMonitor()); > > + > > + int32_t i = PrefToIndex(aPref); > > + if (i < 0 || static_cast<uint32_t>(i) >= ArrayLength(sPrefs)) { > > + DOM_CAMERA_LOGW("Preference '%s' is not tracked by CameraPreferences\n", aPref); > > I assume these log macros are safe to call from any thread. Yes. They wrap PR_LOG macros, which are used everywhere and are threadsafe: http://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/io/prlog.c#432

Asking for a re-review due to changes to remove static constructors. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=a12f92ae8415

Comment on attachment 8441727 [details] [diff] [review] Handle Camera preferences on Main Thread, v5.2 Clearing r? while I work on the try-server leakcheck fail.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=f74e7bb72d46

(P.S. TIL a mutex and condvar leak probably means a Monitor leak.)

Fixed try server memory leaks. Requesting re-review due to significant changes post-r+.

Comment on attachment 8442219 [details] [diff] [review] Handle Camera preferences on Main Thread, v5.3 Review of attachment 8442219 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/camera/CameraPreferences.cpp @@ +198,5 @@ > + DOM_CAMERA_LOGW("Preference '%s' is not a string type\n", aPref); > + return false; > + } > + > + nsCString** s = static_cast<nsCString**>(sPrefs[i].mValue); This cast is wrong, I think, since sPrefs[i].mValue is a StaticAutoPtr<nsCString> I think. It would work at runtime as long as the binary layouts are exactly the same... But I think assuming that is bad. So thinking more about this, why not make mValue a union of a nsCString* and an nsresult*? That would mean we can get rid of all of these casts! And I doubt making these values a StaticAutoPtr<nsCString> will buy us anything.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #37) > Comment on attachment 8442219 [details] [diff] [review] > Handle Camera preferences on Main Thread, v5.3 > > Review of attachment 8442219 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/camera/CameraPreferences.cpp > @@ +198,5 @@ > > + DOM_CAMERA_LOGW("Preference '%s' is not a string type\n", aPref); > > + return false; > > + } > > + > > + nsCString** s = static_cast<nsCString**>(sPrefs[i].mValue); > > This cast is wrong, I think, since sPrefs[i].mValue is a > StaticAutoPtr<nsCString> I think. It would work at runtime as long as the > binary layouts are exactly the same... But I think assuming that is bad. You're right, of course. I missed that. > So thinking more about this, why not make mValue a union of a nsCString* and > an nsresult*? That would mean we can get rid of all of these casts! And I > doubt making these values a StaticAutoPtr<nsCString> will buy us anything. I made that change to simplify the Shutdown() method.

(In reply to comment #38) > > So thinking more about this, why not make mValue a union of a nsCString* and > > an nsresult*? That would mean we can get rid of all of these casts! And I > > doubt making these values a StaticAutoPtr<nsCString> will buy us anything. > > I made that change to simplify the Shutdown() method. Well, you would just need to delete the pointers directly, right? That doesn't seem too bad.

Fix type, use union to remove static_casts. try-server push: https://tbpl.mozilla.org/?tree=Try&rev=5605e150c103

Comment on attachment 8442301 [details] [diff] [review] Handle Camera preferences on Main Thread, v5.4 Review of attachment 8442301 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/camera/CameraPreferences.h @@ +36,5 @@ > + struct Pref { > + const char* const mPref; > + PrefValueType mValueType; > + union { > + void* mAsVoid; This seems to be unused, please remove it.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #41) > > > + void* mAsVoid; > > This seems to be unused, please remove it. It's required to properly initialize the union. union Foo { unsigned int i; const char* c; }; Foo i = { 3 }; // OK Foo c = { "Hello, world!" }; // "error: invalid conversion" ...and since C++ doesn't support designated initializers, stuffing the cached-value pointers into a void* seems to be the only option.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=be558b4cf437

(In reply to comment #42) > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment > #41) > > > > > + void* mAsVoid; > > > > This seems to be unused, please remove it. > > It's required to properly initialize the union. > > union Foo { > unsigned int i; > const char* c; > }; > > Foo i = { 3 }; // OK > Foo c = { "Hello, world!" }; // "error: invalid conversion" > > ...and since C++ doesn't support designated initializers, stuffing the > cached-value pointers into a void* seems to be the only option. Holy moly! OK, then please add a comment describing why it's needed in order to help the next poor soul who gets confused by this like I was. :-)

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #44) > > Holy moly! OK, then please add a comment describing why it's needed in > order to help the next poor soul who gets confused by this like I was. :-) Will do. :) As a point of interest, I tried a patch that _does_ use designated initializers, and _some_ of our platforms support them: https://tbpl.mozilla.org/?tree=Try&rev=fc4cccc48f4d Ones that don't include Windows XP and the B2G Desktop Linux build. (Locally, the B2G ICS emulator build also complains.)

Final patch with requested comments explaining my a 'void*' has to the first member of a union of pointers. No other changes; carrying r+ forward.

There is enough compiler variation here that I'm going to try-soak this one before landing. https://tbpl.mozilla.org/?tree=Try&rev=ecd95c1693df