Last Comment Bug 772987 - Add Static{Auto,Ref}Ptr, like ns{Auto,Ref}Ptr, but without the static initializer
: Add Static{Auto,Ref}Ptr, like ns{Auto,Ref}Ptr, but without the static initial...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on:
Blocks: 773014
  Show dependency treegraph
 
Reported: 2012-07-11 12:20 PDT by Justin Lebar (not reading bugmail)
Modified: 2012-08-28 23:00 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Add Static{Auto,Ref}Ptr. (7.34 KB, patch)
2012-07-11 12:24 PDT, Justin Lebar (not reading bugmail)
benjamin: review+
mh+mozilla: feedback+
Details | Diff | Splinter Review
Part 2: Use Static{Auto,Ref}Ptr where appropriate. (10.33 KB, patch)
2012-07-11 12:24 PDT, Justin Lebar (not reading bugmail)
mounir: review+
Details | Diff | Splinter Review
Part 3: Update comment in ClearOnShutdown. (1.68 KB, patch)
2012-07-11 12:36 PDT, Justin Lebar (not reading bugmail)
mh+mozilla: review+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-07-11 12:20:44 PDT
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.
Comment 1 Justin Lebar (not reading bugmail) 2012-07-11 12:24:01 PDT
Created attachment 641158 [details] [diff] [review]
Part 1: Add Static{Auto,Ref}Ptr.
Comment 2 Justin Lebar (not reading bugmail) 2012-07-11 12:24:26 PDT
Created attachment 641159 [details] [diff] [review]
Part 2: Use Static{Auto,Ref}Ptr where appropriate.

This is mostly hal code, so over to Mounir.
Comment 3 Justin Lebar (not reading bugmail) 2012-07-11 12:25:57 PDT
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 4 Justin Lebar (not reading bugmail) 2012-07-11 12:30:01 PDT
https://tbpl.mozilla.org/?tree=Try&rev=5fc9ae51b2fe
Comment 5 Justin Lebar (not reading bugmail) 2012-07-11 12:36:03 PDT
Created attachment 641165 [details] [diff] [review]
Part 3: Update comment in ClearOnShutdown.
Comment 6 :Ehsan Akhgari 2012-07-11 13:17:12 PDT
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!  ;-)
Comment 7 Justin Lebar (not reading bugmail) 2012-07-11 13:21:05 PDT
> 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.
Comment 8 :Ehsan Akhgari 2012-07-11 13:30:28 PDT
(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 9 Justin Lebar (not reading bugmail) 2012-07-11 13:35:23 PDT
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 10 Justin Lebar (not reading bugmail) 2012-07-11 13:38:52 PDT
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.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-13 14:34:07 PDT
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.
Comment 12 Justin Lebar (not reading bugmail) 2012-07-13 14:43:44 PDT
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.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-07-13 15:17:31 PDT
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.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-07-13 15:19:19 PDT
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.)
Comment 15 Justin Lebar (not reading bugmail) 2012-07-13 15:22:29 PDT
(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.
Comment 16 Justin Lebar (not reading bugmail) 2012-07-26 12:28:30 PDT
https://tbpl.mozilla.org/?tree=Try&rev=9a78335b8413
Comment 17 Justin Lebar (not reading bugmail) 2012-07-26 12:35:58 PDT
(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
Comment 18 Mozilla RelEng Bot 2012-07-26 12:45:25 PDT
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
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-26 15:54:35 PDT
Maybe this should have gone into mfbt...
Comment 20 Justin Lebar (not reading bugmail) 2012-07-27 07:24:23 PDT
Well, the mfbt RefPtr is not compatible with XPCOM objects...
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-27 14:23:43 PDT
(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?
Comment 23 Justin Lebar (not reading bugmail) 2012-07-27 14:38:25 PDT
(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.
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-27 15:41:03 PDT
Yes, in fact, thanks to Bas it was made MSCOM-compatible at the cost of some unsafety.
Comment 25 Jesse Ruderman 2012-08-28 15:40:19 PDT
I'd expect compilers to eliminate static initializers that just set members to zero.  Was that not happening?
Comment 26 Nathan Froyd [:froydnj] 2012-08-28 18:05:09 PDT
(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.)
Comment 27 Justin Lebar (not reading bugmail) 2012-08-28 20:14:15 PDT
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.
Comment 28 Mike Hommey [:glandium] 2012-08-28 23:00:18 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.