1265626 - Clean up CycleCollectedJSRuntime

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

I have some patches to make CycleCollectedJSRuntime nicer and less error-prone.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove CycleCollectedJSRuntime::Initialize()

Making CycleCollectedJSRuntime's constructor fallible (via an nsresult&
outparam) makes the code nicer than having a separate Initialize() function.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Make CycleCollectedJSRuntime::Runtime() return a reference instead of a pointer

This makes it obvious that it's infallible, which allows numerous useless
null-checks to be removed. It also allows a few other minor functions return
references as well.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove now-unnecessary MOZ_ASSERT(mJSRuntime) calls

Now that CycleCollectedJSRuntime no longer has an Initialize() function, it
cannot be left in a partially-initialized state. And mJSRuntime can only be
null if the constructor failed entirely. So there's no need for a mJSRuntime
assertion in every method.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove newXPCJSRuntime()

Now all the XPCJSRuntime initialization can be done in its constructor.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This sort of fallible constructor with an outparam is not something that we do in Mozilla code (I think there are actually zero uses of this pattern in mozilla-central with an nsresult outparam), so I'm not going to sign off on this without a broader discussion on dev-platform.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> This sort of fallible constructor with an outparam is not something that we
> do in Mozilla code (I think there are actually zero uses of this pattern in
> mozilla-central with an nsresult outparam), so I'm not going to sign off on
> this without a broader discussion on dev-platform.

Ok. But without committing yourself to an r+, what do you think of it personally? One reason I posted these patches was to get some preliminary feedback before going to a wider discussion.

Comment 7

[Nicholas Nethercote [inactive]]

(Needinfo for comment 6.)

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't think it's unreasonable.  But can we create a static constructor function (e.g. a Foo* Foo::Create(arg)) that can fail instead?  I would probably prefer that.

Comment 9

[Nicholas Nethercote [inactive]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #8)
> I don't think it's unreasonable.  But can we create a static constructor
> function (e.g. a Foo* Foo::Create(arg)) that can fail instead?  I would
> probably prefer that.

That works for heap-allocated objects but not for stack-allocated objects, and the one WorkerJSRuntime instance is stack-allocated.

Also, even if you do use a factory method, the "ctor + Init" vs. "ctor w/outparam" question still applies *within* the factory method.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That's fair.  You get to choose whether you have allow half constructed objects to exist or brokenly constructed objects.  Not sure that one is better than the other.  Being able to use const and references for member variables is compelling though.

Comment 11

[Nicholas Nethercote [inactive]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #10)
> You get to choose whether you have allow half constructed
> objects to exist or brokenly constructed objects.

Not really...

With ctor+Init you have three possible object states in a well-coded class:

- Half-constructed (i.e. after the constructor runs, but before Init() is called). The dtor doesn't need to handle this.

- Fully-constructed (after successful Init()). The dtor needs to handle this.

- Fully-failed (after failed Init()). The dtor needs to handle this.

With ctor+outparam you have two possible object states in a well-coded class:

- Fully-constructed (after successful construction). The dtor needs to handle this.

- Fully-failed (after failed construction). The dtor needs to handle this.

You can still screw up in two ways in either case.

- If, on failure, you don't appropriately roll back any partial initialization already done (such as leaking things you've allocated). 

- If your dtor doesn't handle both the fully-constructed case (unlikely) and the fully-failed case (as per bug 1265035).

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8742646 [details] [diff] [review]
(part 1) - Remove CycleCollectedJSRuntime::Initialize()

Review of attachment 8742646 [details] [diff] [review]:
-----------------------------------------------------------------

It's been a while, and I don't think anybody raised any particularly serious objections to this pattern.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8742647 [details] [diff] [review]
(part 2) - Make CycleCollectedJSRuntime::Runtime() return a reference instead of a pointer

Review of attachment 8742647 [details] [diff] [review]:
-----------------------------------------------------------------

I don't find this one compelling, sorry.  I didn't check every callsite but at least the vast majority of them end up doing &Runtime() because they actually want the pointer, and we have the Foo() vs GetFoo() convention to indicate that the getter cannot/can return null respectively.

Comment 14

[Nicholas Nethercote [inactive]]

Not worth pursuing further.