635475 - nsIdleServiceDaily can cause pointer corruption

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Marco Bonardo [:mak]]

nsIdleServiceDaily constructor is passing "this" to various other methods that could addref and release it (timer for example), the result I saw locally by making a run on idle is that nsIdleServiceDaily's pointer gets corrupt and we crash badly on idleService initialization.
I have a patch locally to fix this, will attach it here once I split it.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 2

[Shawn Wilsher :sdwilsh]

This is something bad that would could see in real life.

Comment 3

[Shawn Wilsher :sdwilsh]

How does adding Init help us in real life?  We still end up calling that code in the constructor, so don't we have the same problem still?

Comment 4

[Marco Bonardo [:mak]]

the problem is not the idleService constructor, indeed it passes "this" to nsIdleServiceDaily, but nsIdleServiceDaily uses it as a common weak pointer.

The problem is that nsIdleServiceDaily constructor was passing "this" (nsIdleServiceDaily object) to something that instead wants to own it.

Comment 5

[Marco Bonardo [:mak]]

so the scenario is that nsIdleServiceDaily is created, in the constructor it creates a timer that owns it, but we are already idle, we serve it, the timer gets canceled and destroyed and the reference is freed, freeing nsIdleServiceDaily object itself since we are still in the constructor.
As I said, I reproduced this forcing a idle on start to simulate the environment of our talos boxes.
Adding init the reference count becomes 2, when the timer  owns the object.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 514033 [details] [diff] [review]
patch v1.0

r=sdwilsh

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 514033 [details] [diff] [review]
patch v1.0

Well, this still needs approval since softblockers are not pre-approved.

The change is small enough to limit the risk, this removes a possible source of jumps at random memory addresses.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 514033 [details] [diff] [review]
patch v1.0

hm, I'm seeing a second crash in the category cache destructor, I prefer looking at it before proceeding.

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Ah, now this makes sense.  The destructor was invoked by the idle service observers TArray destructor, and it was trying to remove itself from the TArray, causing re-entrance in itself.  The result was a double invokation of the category cache destructor.
Actually, there is no need at all to remove the daily observer from the array, it is kept alive by mDailyIdle and if it's in the array it's destroyed when both mDailyIdle and the array are destroyed. So also tracking whether it's observing through mObservesIdle is pointless.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 514506 [details] [diff] [review]
patch v1.1

r=sdwilsh

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 514506 [details] [diff] [review]
patch v1.1

Ok, this is good for approval, removes possible risk of pointer corruption on startup and some not useful code.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 514506 [details] [diff] [review]
patch v1.1

Isn't this changing an interface and thus not acceptable past add-on compatibility freeze?

Comment 13

[Marco Bonardo [:mak]]

no, this is a pure internal class. it's a helper used internally by the idle service.

Comment 14

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/9833991d7abb