304669 - nsTimerImpl crashes even if called by JS, should me more defensive about pointers

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Giorgio Maone [:ma1]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050815
Firefox/1.0+

This JavaScript snippet crashes Firefox:

Components.classes["@mozilla.org/timer;1"]
    .createInstance(Components.interfaces.nsITimer)
    .initWithCallback(null,1000,0); 

This is pretty obvious (in fact, I've noticed the problem looking at the source
code, rather than actually crashing my browser), because there's no argument
validation in nsTimerImpl::Init[WithXxx]() methods.
While it may be acceptable for InitFuncCallback(), which is meant to be called
by adult C++ programmers, it becomes embarrassing for scriptable methods like
InitWithCallback() and Init().

Also, we NS_RELEASE() the callback in ReleaseCallback(), but we give it away
through a public scriptable property. If a client (e.g. a poorly written plugin)
over-releases this property, we get another crash on next ReleaseCallback()
(e.g. in destructor). Maybe better replacing with NS_IF_RELEASE()?

Comment 1

[Giorgio Maone [:ma1]]

Attachment: Defensive callback management

Comment 2

[timeless]

Comment on attachment 192712 [details] [diff] [review]
Defensive callback management

i'm not technically an xpcom peer, but yes i believe this should be accepted.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 192712 [details] [diff] [review]
Defensive callback management

how can the mCallback.[i|o] be null?  I think the .cpp change is good, but I do
not understand the .h change.

Comment 4

[Giorgio Maone [:ma1]]

(In reply to comment #3)
> (From update of attachment 192712 [details] [diff] [review] [edit])
> how can the mCallback.[i|o] be null?  I think the .cpp change is good, but I do
> not understand the .h change.

Maybe you're right and this is overkill, but since I was ranting about defensive
programming, I digged a bit of talkback data and found a ton of
nsITimerImpl:ReleaseCallback() crap, e.g.
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=8360817


Being all but a XPCOM guru, I'm the last person who can tell you how it happens
, but we both know shit do happen. OTHO, as a quite seasoned Java programmer, I
can tell you it tends to happen in mysterious ways even in the safest
environments, when threads are around, not to mention if a module is doomed by
its own name :)

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Maybe leave the NS_RELEASE's as-is, but add in NS_ASSERTION's to make sure that
they're non-null?  If the member is null, something pretty bad happened, I'd
think, and we don't want to mask it..

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I agree with dougt that the nsTimerImpl.h change should not be needed.  It seems
sensible to maintain the invariant that mCallbackType being set to non-UNKNOWN
implies a non-null callback.  Does your patch not cause us to maintain that
invariant?  If not, could you fix that instead?

Comment 7

[Giorgio Maone [:ma1]]

(In reply to comment #6)
> I agree with dougt that the nsTimerImpl.h change should not be needed.  It seems
> sensible to maintain the invariant that mCallbackType being set to non-UNKNOWN
> implies a non-null callback.  Does your patch not cause us to maintain that
> invariant?  If not, could you fix that instead?

My patch hardly change the extent that invariance is enforced to: as far as I
can see nsITimerImpl already does its best on that front, unless some dark
multithreading synchronization issue is happening (e.g. with the TimerManager
destroying idle timers in concurrence with JS garbage collector), but I'm too
much a newbie to tell. 

Actually, my .cpp patch prevents an early crash, happening as soon as the
callback is  reference counted ( NS_ADDREF(mCallback.[i|o]); ), so it's true
that the forementioned invariance was screwed up, but the whole application was
as well hence it didn't matter ;)

Since there's actual talkback data telling that somehow we manage reach
ReleaseCallback() with invalid pointers (and I tend to blame some external
reference counting issue), I agree with vlad on the assertion thing in the .h
patch, supposing he means "leave NS_IF_RELEASE() as it is" and add a *debug
only* assertion to track bogus (client?) behaviours...

Comment 8

[Giorgio Maone [:ma1]]

Attachment: Argument validation only

Cut down patch (.cpp only, no .h) per dbaron's sr-
Probably the destructor crash do deserve a different bug and more
investigation.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 192947 [details] [diff] [review]
Argument validation only

looks fine.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 192712 [details] [diff] [review]
Defensive callback management

clearing old flag

Comment 11

[timeless]

i presume you need someone to check this in ...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.