Closed
Bug 789130
Opened 12 years ago
Closed 12 years ago
Not removing PHal listeners on abnormal subprocess termination
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Core
Hardware Abstraction Layer (HAL)
Tracking
()
People
(Reporter: cjones, Assigned: cjones)
References
Details
Attachments
(1 file)
STR (intermittent) (1) Load video app or cut the rope (2) |adb shell kill -9 [plugin-container]| (3) Rapidly change device orientation HalParent Register*()s for several notifications, but on crash it doesn't Unregister*().
Comment 1•12 years ago
|
||
> (3) Rapidly change device orientation
Sorry...what happens after this? Does the main process crash?
Updated•12 years ago
|
Version: unspecified → Trunk
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → jones.chris.g
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #658978 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #1) > > (3) Rapidly change device orientation > > Sorry...what happens after this? Does the main process crash? BOOM in main process. uaf
Comment 5•12 years ago
|
||
Comment on attachment 658978 [details] [diff] [review] Unregister hal observers on HalParent::ActorDestroy(), and make Unregister*() functions resilient to unregister-of-not-registered observers >diff --git a/hal/Hal.cpp b/hal/Hal.cpp >@@ -180,40 +180,29 @@ public: > mObservers->AddObserver(aObserver); > > if (mObservers->Length() == 1) { > EnableNotifications(); > } > } > > void RemoveObserver(Observer<InfoType>* aObserver) { >- // If mObservers is null, that means there are no observers. >- // In addition, if RemoveObserver() returns false, that means we didn't >- // find the observer. >- // In both cases, that is a logical error we want to make sure the developer >- // notices. >- >- MOZ_ASSERT(mObservers); All that arguing, and now we're deleting the assertion! :) >diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp >+ virtual void >+ ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE >+ { >+ // NB: you *must* unconditionally unregister your observer here, >+ // if it *may* be registered below. >+ hal::UnregisterBatteryObserver(this); >+ hal::UnregisterNetworkObserver(this); >+ hal::UnregisterScreenConfigurationObserver(this); >+ for (int32_t sensor = SENSOR_UNKNOWN + 1; >+ sensor < NUM_SENSOR_TYPE; ++sensor) { >+ hal::UnregisterSensorObserver(SensorType(sensor), this); >+ } >+ hal::UnregisterWakeLockObserver(this); What about UnregisterSwitchObserver? (I wonder if there's some way to assert this, so we don't regress this method.)
Attachment #658978 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #5) > Comment on attachment 658978 [details] [diff] [review] > Unregister hal observers on HalParent::ActorDestroy(), and make > Unregister*() functions resilient to unregister-of-not-registered observers > > >diff --git a/hal/Hal.cpp b/hal/Hal.cpp > >@@ -180,40 +180,29 @@ public: > > mObservers->AddObserver(aObserver); > > > > if (mObservers->Length() == 1) { > > EnableNotifications(); > > } > > } > > > > void RemoveObserver(Observer<InfoType>* aObserver) { > >- // If mObservers is null, that means there are no observers. > >- // In addition, if RemoveObserver() returns false, that means we didn't > >- // find the observer. > >- // In both cases, that is a logical error we want to make sure the developer > >- // notices. > >- > >- MOZ_ASSERT(mObservers); > > All that arguing, and now we're deleting the assertion! :) > We have a strong use case now where the overhead of client logic to maintain "registered or not" is higher than making the hal API softer. We didn't have that at the time. I don't regret the assertion. It's caught real bugs. > >diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp > >+ virtual void > >+ ActorDestroy(ActorDestroyReason aWhy) MOZ_OVERRIDE > >+ { > >+ // NB: you *must* unconditionally unregister your observer here, > >+ // if it *may* be registered below. > >+ hal::UnregisterBatteryObserver(this); > >+ hal::UnregisterNetworkObserver(this); > >+ hal::UnregisterScreenConfigurationObserver(this); > >+ for (int32_t sensor = SENSOR_UNKNOWN + 1; > >+ sensor < NUM_SENSOR_TYPE; ++sensor) { > >+ hal::UnregisterSensorObserver(SensorType(sensor), this); > >+ } > >+ hal::UnregisterWakeLockObserver(this); > > What about UnregisterSwitchObserver? > > (I wonder if there's some way to assert this, so we don't regress this > method.) Since allowing content to register for switch events is a security policy change, it will need sr and I trust sr's here to be aware of unregister. But I don't have a strong opinion either way.
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc088d2b2006
Comment 8•12 years ago
|
||
Surely a rogue content process could call RegisterSwitchObserver()? Anyway, it strikes me as super-weird to have a comment saying "we must unconditionally register all the things" and then intentionally leave one thing out, even if the omission is safe.
Comment 9•12 years ago
|
||
> Surely a rogue content process could call RegisterSwitchObserver()?
Maybe the point is "I don't care about content processes safely crashing the parent process"? I still don't get it, though. Just unregister the thing and we're safe, right?
Assignee | ||
Comment 10•12 years ago
|
||
A rogue content process calling RegisterSwitchObserver() would be entirely ignored by the parent process. In fact, the content process would be killed off. I may not be understanding what you're referring to.
Assignee | ||
Comment 11•12 years ago
|
||
I thought you meant, "In a future where we allow that, we should add a static check now to ensure we don't fuck up the Unregister() that would be needed."
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc088d2b2006
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•