The default bug view has changed. See this FAQ.

Add Static{Auto,Ref}Ptr, like ns{Auto,Ref}Ptr, but without the static initializer

RESOLVED FIXED in mozilla17

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
(Assignee)

Comment 1

5 years ago
Created attachment 641158 [details] [diff] [review]
Part 1: Add Static{Auto,Ref}Ptr.
Attachment #641158 - Flags: review?(mh+mozilla)
(Assignee)

Comment 2

5 years ago
Created attachment 641159 [details] [diff] [review]
Part 2: Use Static{Auto,Ref}Ptr where appropriate.

This is mostly hal code, so over to Mounir.
Attachment #641159 - Flags: review?(mounir)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #641158 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Attachment #641159 - Flags: review?(mounir)
(Assignee)

Comment 4

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=5fc9ae51b2fe
(Assignee)

Comment 5

5 years ago
Created attachment 641165 [details] [diff] [review]
Part 3: Update comment in ClearOnShutdown.
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!  ;-)
(Assignee)

Comment 7

5 years ago
> 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.  :-)
(Assignee)

Updated

5 years ago
Blocks: 773014
(Assignee)

Comment 9

5 years ago
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.
Attachment #641158 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Attachment #641159 - Flags: review?(mounir)
(Assignee)

Comment 10

5 years ago
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.
Attachment #641165 - Flags: review?(mh+mozilla)
Attachment #641159 - Flags: review?(mounir) → review+
Attachment #641158 - Flags: review?(mh+mozilla)
Attachment #641158 - Flags: review?(benjamin)
Attachment #641158 - Flags: feedback+
Attachment #641165 - Flags: review?(mh+mozilla) → review+
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.
(Assignee)

Comment 12

5 years ago
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.)
(Assignee)

Comment 15

5 years ago
(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.
Attachment #641158 - Flags: review?(benjamin) → review+
(Assignee)

Comment 16

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=9a78335b8413
(Assignee)

Comment 17

5 years ago
(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

5 years ago
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...
(Assignee)

Comment 20

5 years ago
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(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?
(Assignee)

Comment 23

5 years ago
(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.

Comment 25

5 years ago
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.)
(Assignee)

Comment 27

5 years ago
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.
You need to log in before you can comment on or make changes to this bug.