Add compulsory Init functions to AutoEntryScript similar to those for AutoJSAPI

NEW
Unassigned

Status

()

Core
DOM
4 years ago
3 years ago

People

(Reporter: bobowen, Unassigned)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

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

Updated

3 years ago
Assignee: nobody → bobowencode
(Reporter)

Comment 1

3 years ago
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?
Flags: needinfo?(bobbyholley)
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?
Depends on: 971673
Flags: needinfo?(bobbyholley) → needinfo?(jimb)
(Reporter)

Comment 3

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

3 years ago
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.
Flags: needinfo?(jimb)

Comment 5

3 years ago
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?
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

3 years ago
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

3 years ago
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.
(Reporter)

Comment 9

3 years ago
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.
(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.
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.
Flags: needinfo?(jimb)

Comment 12

3 years ago
I can do this, but I'm going to be on vacation from this Friday Aug 22 through the next Tuesday, Aug 26.
Flags: needinfo?(jimb)
(Reporter)

Comment 13

3 years ago
I think this will be done as part of bug 971673 now.
Assignee: bobowen.code → nobody
You need to log in before you can comment on or make changes to this bug.