It's silly that, when we want static variables, we have to choose between startup speed (raw pointers) and safety (using static ns{Auto,Ref}Ptr, which creates an additional static initializer). I have a patch to add clones of ns{Auto,Ref}Ptr which don't add static initializers.

This is mostly hal code, so over to Mounir.

Actually, I should probably hold off on the reviews until after I get try results back, because I have a hunch something isn't going to compile somewhere.

Comment on attachment 641158 [details] [diff] [review] Part 1: Add Static{Auto,Ref}Ptr. Can you please file a static analysis bug to make us actually check that these classes are only used in the global scope? I wouldn't trust people with this kind of power even for a second! ;-)

> I wouldn't trust people with this kind of power even for a second! ;-) There's a dynamic (debug-time) assertion that mRawPtr is 0, during the constructor. So hopefully that will catch some cases. Static analysis would be great, though.

(In reply to comment #7) > > I wouldn't trust people with this kind of power even for a second! ;-) > > There's a dynamic (debug-time) assertion that mRawPtr is 0, during the > constructor. So hopefully that will catch some cases. Ah, hmm, that should take care of a whole bunch of cases, yes. But I mean, once we get NS_STACK_CLASS working (which is close if I understand things correctly), this should be easy. :-)

Comment on attachment 641158 [details] [diff] [review] Part 1: Add Static{Auto,Ref}Ptr. try is pretty horked, with no recovery in sight, so I guess I'll just use m-i as my tryserver.

Comment on attachment 641165 [details] [diff] [review] Part 3: Update comment in ClearOnShutdown. I'd normally ask Waldo to review this comment change, but he's busy.

You might be able to ~export http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsTraceRefcntImpl.cpp#133 and use it to assert that these helpers are *only* constructed/destructed from ~static contexts.

Seems like that would only work in builds with --enable-trace-malloc (which isn't the default with debug builds? Or is it?). In any case, I could certainly do something similar. The trick is hearing that we're starting up very early in the game, and hearing that we're shutting down very late in the game.

Wouldn't it be less maintenance burden to make these base classes of nsAutoPtr / nsRefPtr and move all methods except for the destructor to them? Then there'd be no duplication of code, they'd provide the same API, and any future additions would automatically apply to both.

er, destructor and constructor. (You'd also need to add a second protected constructor with a funky signature for the derived class to call, to avoid the assert.)

(In reply to David Baron [:dbaron] from comment #13) > Wouldn't it be less maintenance burden to make these base classes of > nsAutoPtr / nsRefPtr and move all methods except for the destructor to them? > Then there'd be no duplication of code, they'd provide the same API, and any > future additions would automatically apply to both. I considered that, but I rejected it because I'd still have to duplicate a bunch of operators on nsAutoPtr which return nsAutoPtr&, and fix similar problems. (Maybe there's a way around these using C++-fu that I do not yet have.) I'm not really bothered that you can't do getter_AddRefs() on a StaticRefPtr, personally.

(In reply to Justin Lebar [:jlebar] (PTO July 26) from comment #16) > https://tbpl.mozilla.org/?tree=Try&rev=9a78335b8413 Actually, scratch that; I already had a basically-working test run in comment 4. http://hg.mozilla.org/integration/mozilla-inbound/rev/b9a1f5cae6ea http://hg.mozilla.org/integration/mozilla-inbound/rev/d5bc3b732229 http://hg.mozilla.org/integration/mozilla-inbound/rev/7517b13c4d57

Try run for 9a78335b8413 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=9a78335b8413 Results (out of 17 total builds): exception: 17 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-9a78335b8413

Maybe this should have gone into mfbt...

Well, the mfbt RefPtr is not compatible with XPCOM objects...

https://hg.mozilla.org/mozilla-central/rev/b9a1f5cae6ea https://hg.mozilla.org/mozilla-central/rev/d5bc3b732229 https://hg.mozilla.org/mozilla-central/rev/7517b13c4d57

(In reply to Justin Lebar [:jlebar] from comment #20) > Well, the mfbt RefPtr is not compatible with XPCOM objects... That's news to me... why?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22) > (In reply to Justin Lebar [:jlebar] from comment #20) > > Well, the mfbt RefPtr is not compatible with XPCOM objects... > > That's news to me... why? Actually, I'm totally incorrect.

Yes, in fact, thanks to Bas it was made MSCOM-compatible at the cost of some unsafety.

I'd expect compilers to eliminate static initializers that just set members to zero. Was that not happening?

(In reply to Jesse Ruderman from comment #25) > I'd expect compilers to eliminate static initializers that just set members > to zero. Was that not happening? GCC, at least, does not do this. (Long-standing PR.)

There's also the problem of non-trivial static /destructors/, which nsRefPtr does have. And I vaguely recall something awful like the presence of a non-trivial static destructor requires the presence of a static constructor.

(In reply to Justin Lebar [:jlebar] from comment #27) > There's also the problem of non-trivial static /destructors/, which nsRefPtr > does have. And I vaguely recall something awful like the presence of a > non-trivial static destructor requires the presence of a static constructor. Indeed, static destructors are registered with __cxa_atexit in a static constructor. Note that this one applies to MSVC too, contrary to the other case mentioned.