1027553 - Add compulsory Init functions to AutoEntryScript similar to those for AutoJSAPI

Status: ASSIGNED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Bob Owen (:bobowen)]

Change AutoEntryScript to use the same pattern of no-op (as far as AutoJSAPI is concerned at least) constructor followed by compulsory Init function.

Comment 1

[Bob Owen (:bobowen)]

This is complicated by the fact that AutoEntryScript is also a ScriptSettingsStackEntry.
So unless we change ScriptSettingsStackEntry, then our no-op (for AutoJSAPI) constructor will still create and push the ScriptSettingsStackEntry.

Given the assertions in ScriptSettingsStackEntry's constructor, I don't think the Init could currently fail.
However, if the Init did fail and the code returns it would mean we push and pop from the ScriptSettingsStack for no reason.
If the code tried to continue, even though the AutoJSAPI part has not been initialised, we would still have an entry on the stack, which I imagine might cause some debugging confusion.

It would also be odd because we'd need an nsIGlobalObject for the contructor and one could be passed to the Init as well.

Bobby: How do you envisage this working?

Comment 2

[Bobby Holley (:bholley)]

It seems like we shouldn't push the ScriptSettingsStackEntry until Init() is called.

jimb is redesigning the Script Settings Stack stuff in bug 971673. It might make sense to hold off on this until he's done with that. Jim, is that happening soon-ish? Any thoughts on this?

Comment 3

[Bob Owen (:bobowen)]

(In reply to Bobby Holley (:bholley) from comment #2)
> It seems like we shouldn't push the ScriptSettingsStackEntry until Init() is
> called.

Yeah, that was my conclusion, but it means quite a lot more changes, so I thought I'd get your view first.

I guess we'd need to drop the inheritance from ScriptSettingsStackEntry and have this as a Maybed member variable.
Also, would we need to consider not publicly inheriting from AutoJSAPI, to prevent an Init being called that doesn't push a ScriptSettingsStackEntry?

Anyway, as you say below, probably best to wait until jimb has finished his redesign.

Oh ... and all this reminds me of the question over the Script Settings Stack stuff I mentioned on IRC a few weeks ago.

This very much depends on how the stack will end up being used and how performance sensitive it is ...

If we are likely to be reading the stack (for an entry global), more often than we push to it, would it be worth considering having a separate stack to which only AutoEntryScript pushes and pops.
That way we wouldn't have to hunt down the stack each time.

We could assert that the tops are the same before AutoEntryScript pops and possibly other things as well, to ensure they are always in sync.

Just a thought.
I notice at the moment that only CallbackObject uses AutoIncumbentScript and this always creates an AutoEntryScript just before, so unless it will be used elsewhere then it probably wouldn't be worth it.

> jimb is redesigning the Script Settings Stack stuff in bug 971673. It might
> make sense to hold off on this until he's done with that. Jim, is that
> happening soon-ish? Any thoughts on this?

I've been trying to follow this. :-)

I've got some time this weekend that I'm hoping to use to make more progress on bug 951991.
I'll concentrate on the ones that I don't think require AutoEntryScript first.

Comment 4

[Jim Blandy :jimb]

Yeah, that's my current priority. jorendorff has asked for a bunch of changes to the patch I proposed, and I'm working on those now.

The separation of construction from activation is a surprise. One nice thing about a MOZ_STACK_CLASS RAII class is that its lifetime has a very simple relationship to the lifetimes of all JS stack frames: they either completely include, or are completely included by, each other. Naturally, once you separate out initialization, that invariant depends on usage patterns.

Naturally, Maybe<...> also breaks these lifetime rules. It should be an error to instantiate Maybe with a MOZ_STACK_CLASS type, because the assumptions you want to make about MOZ_STACK_CLASS just don't hold any more.

There's really no reason to force you guys to use Maybe<...>: that's just me trying to capture the correct usage pattern in the type (RAII) and then you guys trying to escape it ("Well, it's actually 'Resource Acquisition Is Init' now..."). I ought to just give you the API you really want.

Comment 5

[Jim Blandy :jimb]

It's a shame. I was going to be able to *simplify* your guys' code while *fixing* a bunch of bugs, all with static checks ensuring correct usage.

What's the win of an Init method, exactly? Fallibility?

Comment 6

[Bobby Holley (:bholley)]

Yes, fallibility. It lets callers pass in nsIGlobalObjects and nsPIDOMWindows whose ->GetGlobalJSObject() might return null (i.e. they've been unlinked) without forcing every caller to do that checking.

If it screws up your entire design though, we could reserve those semantics for AutoJSAPI and not implement them for AutoEntryScript. There are fewer of those, so we _could_ just force all callers to null-check. But it's annoying. And it's not totally clear to me why it would, so long as we just required that the initialization always happened immediately after construction.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

If possible, please have the Init() also in AutoEntryScript. Consistency and simplicity in these APIs which deal with black-magic would be very welcome.

Comment 8

[Jim Blandy :jimb]

It's not critical to the design, by any stretch. If we can leverage the guaranteed properties of types, then we should, but that's only one technique out of many, and it needs to be weighed against the costs. Consistency is a big win, too.

Comment 9

[Bob Owen (:bobowen)]

While it's nice to have a fallible Init function, to save boiler plate null checking, I don't think it's crucial.
(Not for the first time in Gecko code, I find myself yearning for exception handling. :-) )

Given that the AutoEntryScript is a ScriptSettingsStackEntry (as well as an AutoJSAPI) and this facet of it may not lend itself to an Init, I think it's reasonable to have the AutoEntryScript auto-initialise its AutoJSAPI parentage.
As long as we make it clear that's what happens (and why) in the header file.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Bob Owen (:bobowen) from comment #9)
> While it's nice to have a fallible Init function, to save boiler plate null
> checking, I don't think it's crucial.
> (Not for the first time in Gecko code, I find myself yearning for exception
> handling. :-) )

Yeah. But I agree with smaug that consistency between the APIs is also probably a Very Good Thing. If jimb can find a way to design it without compromising too much, I think it's worth having it be a little more complicated under the hood on the ScriptSettings.cpp-side in order to make things simpler for consumers.

Comment 11

[Bobby Holley (:bholley)]

Jimb was talking about taking this bug as a prereq to bug 971673. NeedInfoing him to make sure he and Bob coordinate and don't duplicate work.

Comment 12

[Jim Blandy :jimb]

I can do this, but I'm going to be on vacation from this Friday Aug 22 through the next Tuesday, Aug 26.

Comment 13

[Bob Owen (:bobowen)]

I think this will be done as part of bug 971673 now.

Comment 14

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven't been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

We've had a number of instances come up where the additional global validity checking (like nsIGlobalObject::IsDying) on an Init method would have been helpful, so I'm promoting this back up to a P3. A specific set of comments by :smaug on the topic:

• https://bugzilla.mozilla.org/show_bug.cgi?id=1756172#c16
• https://phabricator.services.mozilla.com/D141719#4632859

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

(propagating assignee from bug 1761278 which I duped to this)