1182316 - Add assertions all over nsGlobalWindow enforcing which entry points should be used on inner and outer windows

Status: RESOLVED FIXED

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

Description

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

The basic idea here is to move forwarding macros back to the XPCOM entry points.  WebIDL entry points should be inner only, and implementations that belong on the outer are now separate functions that we forward to.  A handful of callsites elsewhere in Gecko need to be fixed up to accomodate this.

Comment 1

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

Attachment: Part 1: Remove FORWARD_TO_INNER_OR_THROW

All WebIDL entry points should be inner only anyways, so move forwarding back to the XPCOM versions where appropriate.

Comment 2

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

Attachment: Part 2: Rework FORWARD_TO_OUTER_OR_THROW

This makes FORWARD_TO_OUTER_OR_THROW assert that we're already in an inner, and splits a number of functions into WebIDL entry point Foo() and implementation FooOuter() that lives on the outer window.

Comment 3

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

Attachment: Part 3: Miscellaneous assertions, remove nsIDOMJSWindow.

Comment 4

[Peter Van der Beken [:peterv]]

Comment on attachment 8631904 [details] [diff] [review]
Part 1: Remove FORWARD_TO_INNER_OR_THROW

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

I'm a bit torn, it looks safe but I'm worried that we might miss some callers. Maybe it'd make sense to replace the forwards with MOZ_RELEASE_ASSERT? The assert should all eventually go away once you do the real split, right?

Comment 5

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

Yeah, it'll all get scrapped eventually. This is just to segregate the entry points for future splitting.

The XPCOM forwards need to remain for now. There's a bunch of code that depends on them.

Comment 6

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

We could make these assertions MOZ_RELEASE_ASSERT though.  I didn't do it because I was worried about perf.  What do you think?

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 8631908 [details] [diff] [review]
Part 2: Rework FORWARD_TO_OUTER_OR_THROW

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

I find that the Outer suffix leads to weird names, I think I slightly prefer something like a OnOuter suffix or a Outer prefix. But I don't care much given that these should also be temporary, right?

::: dom/base/nsGlobalWindow.cpp
@@ +6014,5 @@
> +nsGlobalWindow::GetScrollMaxXY(int32_t* aScrollMaxX, int32_t* aScrollMaxY,
> +                               ErrorResult& aError)
> +{
> +  FORWARD_TO_OUTER_OR_THROW(GetScrollMaxXYOuter, (aScrollMaxX, aScrollMaxY),
> +                          aError, );

Whitespace is off.

@@ -7974,2 @@
>  {
> -  FORWARD_TO_OUTER_OR_THROW(GetFrames, (aError), aError, nullptr);

Should probably replace this with a IsOuterWindow assert.

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 8631909 [details] [diff] [review]
Part 3: Miscellaneous assertions, remove nsIDOMJSWindow.

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

::: dom/base/nsGlobalWindow.cpp
@@ +4257,3 @@
>  
>    nsDOMWindowList* windows = GetWindowList();
>    NS_ENSURE_TRUE(windows, nullptr);

Hmm, we're not setting aFound anymore sometimes. However, it looks like that's actually unused anyway, so maybe we could just remove it while we're changing the signature anyway?

@@ +4261,5 @@
>    return windows->IndexedGetter(aIndex, aFound);
>  }
>  
> +already_AddRefed<nsIDOMWindow>
> +nsGlobalWindow::IndexedGetter(uint32_t aIndex, bool& aFound)

Can't you actually remove this? If so then you could rename IndexedGetterOuter to IndexedGetter again.

Comment 9

[Peter Van der Beken [:peterv]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> We could make these assertions MOZ_RELEASE_ASSERT though.  I didn't do it
> because I was worried about perf.  What do you think?

Aren't we already doing the check anyway for things called from bindings? If so, then I don't think I'd worry so much about perf.

Comment 10

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

(In reply to Peter Van der Beken [:peterv] from comment #8)
> Comment on attachment 8631909 [details] [diff] [review]
> Part 3: Miscellaneous assertions, remove nsIDOMJSWindow.
> 
> Review of attachment 8631909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsGlobalWindow.cpp
> @@ +4257,3 @@
> >  
> >    nsDOMWindowList* windows = GetWindowList();
> >    NS_ENSURE_TRUE(windows, nullptr);
> 
> Hmm, we're not setting aFound anymore sometimes. However, it looks like
> that's actually unused anyway, so maybe we could just remove it while we're
> changing the signature anyway?

Done.

> @@ +4261,5 @@
> >    return windows->IndexedGetter(aIndex, aFound);
> >  }
> >  
> > +already_AddRefed<nsIDOMWindow>
> > +nsGlobalWindow::IndexedGetter(uint32_t aIndex, bool& aFound)
> 
> Can't you actually remove this? If so then you could rename
> IndexedGetterOuter to IndexedGetter again.

It gets called on the inner via xrays.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a7ab1cb95bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bb87a230841
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3c9358b570

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a7ab1cb95bb
https://hg.mozilla.org/mozilla-central/rev/7bb87a230841
https://hg.mozilla.org/mozilla-central/rev/cd3c9358b570

Comment 17

[Eric Rahm [:erahm]]

I think we may need to back this out until bug 185793 is resolved, this is currently the cause of the #2 most verbose warning during testing.

Comment 18

[Eric Rahm [:erahm]]

* bug 1183888 that is

Comment 19

[Boris Zbarsky [:bzbarsky]]

No.  Do not back this out.  Let's just fix bug 1183888.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Er, I assume you _really_ meant bug 1185793.