Bob Owen (:bobowen) Assignee
	Description • 11 years ago

Going to hit all of the remaining cases in nsGlobalWindow for this batch.

See bug 951991 for details.

	Bob Owen (:bobowen) Assignee
	Comment 1 • 11 years ago

Expecting an r- here. :-)

I seem to have a catch-22 here, where I can't create the AutoEntryScript, because I need the global to have been created on the inner window, but the creation needs the context to be pushed.

I tried just using a JSAutoRequest, but that doesn't work.
I don't think a JSAutoCompartment would help as the original nsCxPusher doesn't create one as far as I can tell and I'm not sure whose compartment I would enter ... given the global doesn't exist.

I'm also not sure this is a C (Gecko specific) it could be a B, in which case I think the spec would need changing here (at step 1):
http://www.whatwg.org/specs/web-apps/current-work/#initialize-the-document-object

Apart from that, I'm really confident. :-)

	Bob Owen (:bobowen) Assignee
	Comment 2 • 11 years ago

I don't think we need an AutoEntryScript, because the context should already have been pushed.
I dare say adding new JSContext* parameters here is not the object, but using an AutoJSContext seemed a bit presumptuous as we probably do care which context we're using.
So, passing it in and asserting that it is already pushed seemed the best plan.

Of course I could just grab the context and assert that it is pushed, but that seemed a bit random.

	Bobby Holley (:bholley)
	Comment 3 • 11 years ago

Comment on attachment 8397209 [details] [diff] [review]
Part 1: Replace nxCxPusher in nsGlobalWindow::SetNewDocument v1

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

Hm, do we need an entry script at all here? What if we just did an AutoJSContext, and entered the compartment once we had the inner window? That is to say, this may be a D.

	Bobby Holley (:bholley)
	Comment 4 • 11 years ago

Comment on attachment 8397289 [details] [diff] [review]
Part 2: Replace AutoPushJSContext in nsGlobalWindow::CreateOuterObject v1

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

::: dom/base/nsGlobalWindow.cpp
@@ +2137,5 @@
>  
>  nsresult
> +nsGlobalWindow::CreateOuterObject(JSContext* aCx, nsGlobalWindow* aNewInner)
> +{
> +  MOZ_ASSERT(aCx == nsContentUtils::GetCurrentJSContext());

This assertion isn't necessary - we have the invariant that any usage of a non-stack-top cx must be accompanied by a push, and the JS engine will assert it for us (see bug 977340).

So we don't need to pass the JSContext here - AutoJSContext should always be equivalent.

@@ +2148,5 @@
>    }
>  
>    js::SetProxyExtra(outer, 0, js::PrivateValue(ToSupports(this)));
>  
> +  JSAutoCompartment ac(aCx, outer);

Having the JSAutoCompartment here is pretty nonsensical (I know this predates your patch). Before creating the outer, we should either assert js::IsObjectInContextCompartment(cx, global), or just JSAutoCompartment ac(cx, global). The former is probably sufficient assuming we do in, in fact, enter the compartment in the caller.

	Bob Owen (:bobowen) Assignee
	Comment 5 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #3)
> Comment on attachment 8397209 [details] [diff] [review]
> Part 1: Replace nxCxPusher in nsGlobalWindow::SetNewDocument v1
> 
> Review of attachment 8397209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm, do we need an entry script at all here? What if we just did an
> AutoJSContext, and entered the compartment once we had the inner window?
> That is to say, this may be a D.

Ah, I'd thought with all the things it was doing with the context below we would need an entry script.
I guess that most of it is just setting stuff up on standard objects.
So, do we only need a script entry point when we are (or could) run "user" script, whether privileged or from content?
If so, the only use of the context that looks like it might do that is the call to JS_FireOnNewGlobalObject near the end of the function.
It looks like this calls functions on debuggers that are registered to be notified of this.

Anyway, if I use an AutoJSContext, it all still seems to work.
This gives you a different context at least the first time through, the SafeJSContext.
So, I've dug down into the usages of the context (for my own understanding) in this function and it mainly seems to be for:
* Rooting
* Compartment tracking
* Accessing other things like the runtime, which presumably will be the same.

I'm assuming this is why it doesn't matter which context we get and also why we want to get rid of them, because they're mostly just used for convenience and it only matters which ones we have in certain circumstances.

Also, I don't think we need to enter the compartment of the global when we have the inner window.
All of the things that get called before we enter it anyway further down, seem to enter any compartments they need to already.
Further down it uses the mJSObject on the outer, but I think this is in the same compartment anyway.

	Bob Owen (:bobowen) Assignee
	Comment 6 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #4)

> So we don't need to pass the JSContext here - AutoJSContext should always be
> equivalent.

Ah, I think I see, instead of passing it explicitly and assuming that our caller will have pushed it, we grab the one on the stack and assume that if our caller cared which one we use, it will have pushed it.
So, we have less to clean up.

> 
> @@ +2148,5 @@
> >    }
> >  
> >    js::SetProxyExtra(outer, 0, js::PrivateValue(ToSupports(this)));
> >  
> > +  JSAutoCompartment ac(aCx, outer);
> 
> Having the JSAutoCompartment here is pretty nonsensical (I know this
> predates your patch). Before creating the outer, we should either assert
> js::IsObjectInContextCompartment(cx, global), or just JSAutoCompartment
> ac(cx, global). The former is probably sufficient assuming we do in, in
> fact, enter the compartment in the caller.

I saw that and thought it was a bit odd, given that it didn't appear to be doing anything that needed it.
NewOuterWindowProxy already enters the compartment of the global that is passed in, so I think we can just get rid of this line and not worry about it.

	Bob Owen (:bobowen) Assignee
	Comment 7 • 11 years ago

(In reply to Bob Owen (:bobowen) from comment #5)

> Also, I don't think we need to enter the compartment of the global when we
> have the inner window.

Forgot to mention that I've debugged through the original code and this didn't appear to be entering any compartments with the |cxPusher.Push|, so this seems to confirm it.

	Bobby Holley (:bholley)
	Comment 8 • 11 years ago

I need to think (and talk to Boris) about the SetNewDocument case. I'll try to come up with an answer soon.

	Bob Owen (:bobowen) Assignee
	Comment 9 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #8)
> I need to think (and talk to Boris) about the SetNewDocument case. I'll try
> to come up with an answer soon.

OK, thanks.
I don't think the SetNewDocument case affects the others, so I'll cary on with them.

nsJSContext::SetProperty pushes its own context, so I think we can just have an AutoJSContext here.
I think we might be able to do the same in SetProperty, but that can wait for its own patch.

Also, got rid of the confusing (at least to me) extra |GetContextInternal|, as unless I'm mistaken it will always give us the one we already have.

	Bob Owen (:bobowen) Assignee
	Comment 10 • 11 years ago

I don't think that we are actually running any script here, but it looks like there is quite a lot of potential error reporting in the deserialisation of the structured clone data.
So, I think this is one of the cases where we need to push for error reporting.

	Bob Owen (:bobowen) Assignee
	Comment 11 • 11 years ago

Not sure if I'm completely mis-reading these, but this looks like another where the context is just being used for error reporting.

	Bob Owen (:bobowen) Assignee
	Comment 12 • 11 years ago

Again, could be missing something here, but I don't quite see why we need to push the context just to see if it is not null.
In fact I'm not quite sure why the contexts are being checked at all.

try push with all of these patches:
https://tbpl.mozilla.org/?tree=Try&rev=27882ae386b9

	Bob Owen (:bobowen) Assignee
	Comment 13 • 11 years ago

(In reply to Bob Owen (:bobowen) from comment #12)

> try push with all of these patches:
> https://tbpl.mozilla.org/?tree=Try&rev=27882ae386b9

The failures in this try push seem to be around exception handling, looks like we might need the correct context pushed.
I did some experiments (by just dropping in an AutoPushJSContextForErrorReporting) to try and isolate the code that needed it and it seems to be the CreateNativeGlobalForInner, so I tried a try push with this as a test:
https://tbpl.mozilla.org/?tree=Try&rev=34c4342edd4f

I think this might be because XPCWrappedNative::WrapNewGlobal uses AutoJSContext and maybe this needs the specific context to be pushed.

	Bobby Holley (:bholley)
	Comment 14 • 11 years ago

Comment on attachment 8397893 [details] [diff] [review]
Part 1: Replace nxCxPusher in nsGlobalWindow::SetNewDocument v2

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

Now that bug 988383 is sorted, I think this should be an AutoJSAPI to begin, and then an AutoEntryScript before firing OnNewGlobalObject. Sound good?

	Bobby Holley (:bholley)
	Comment 15 • 11 years ago

Comment on attachment 8397907 [details] [diff] [review]
Part 2: Replace AutoPushJSContext in nsGlobalWindow::CreateOuterObject v2

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

::: dom/base/nsGlobalWindow.cpp
@@ +2142,4 @@
>  
>    JS::Rooted<JSObject*> global(cx, aNewInner->FastGetGlobalJSObject());
> +  JS::Rooted<JSObject*> outer(cx, NewOuterWindowProxy(cx, global,
> +                              IsChromeWindow()));

Nit - I think that IsChromeWindow() should be aligned with the open-paren of NewOuterWindowProxy. But does the indentation need to change here at all?

	Bobby Holley (:bholley)
	Comment 16 • 11 years ago

Comment on attachment 8398476 [details] [diff] [review]
Part 4: Replace AutoPushJSContext in PostMessageEvent::Run v1

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

::: dom/base/nsGlobalWindow.cpp
@@ +7795,5 @@
>                      "should have been passed an outer window!");
>  
>    // Get the JSContext for the target window
>    nsIScriptContext* scriptContext = mTargetWindow->GetContext();
> +  AutoPushJSContextForErrorReporting cx(scriptContext

This should now be AutoJSAPIWithErrorsReportedToWindow(scriptContext);

	Bobby Holley (:bholley)
	Comment 17 • 11 years ago

Comment on attachment 8398482 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in nsGlobalWindow::SecurityCheckURL v1

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

This is really sensitive code - in fact, this was the function that nailed us in pwn2own this year - see bug 982906. So we should tread carefully here.

In particular, I don't think this patch is right, because the push will affect the subject principal, which is what nsScriptSecurityManager::CheckLoadURIFromScript is based on. Can you file this as a separate bug and CC me and Boris? We'll try to figure out what this function actually needs to be doing.

	Bobby Holley (:bholley)
	Comment 18 • 11 years ago

Comment on attachment 8398533 [details] [diff] [review]
Part 6: Replace AutoPushJSContext in nsGlobalWindow::GetMessageManager v1

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

Nice.

	Bob Owen (:bobowen) Assignee
	Comment 19 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #14)
> Comment on attachment 8397893 [details] [diff] [review]
> Part 1: Replace nxCxPusher in nsGlobalWindow::SetNewDocument v2
> 
> Review of attachment 8397893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Now that bug 988383 is sorted, I think this should be an AutoJSAPI to begin,
> and then an AutoEntryScript before firing OnNewGlobalObject. Sound good?

An AutoJSAPI, doesn't seem to be enough here.
This goes back to my findings in Comment 13.
WrapNewGlobal does various things with the context it grabs, but JS_InitStandardClasses [1] does a setDefaultCompartmentObjectIfUnset() on the context, so I'm not sure if this could be the issue.

We could use AutoJSAPIWithErrorsReportedToWindow instead this appears to work locally and I've got a try push running (with Part 4 changed to the new world as well):
https://tbpl.mozilla.org/?tree=Try&rev=0e5c61445868

We could just drop in a AutoJSAPIWithErrorsReportedToWindow before the call to CreateNativeGlobalForInner, but this seems to be getting a bit convoluted.

Or we could just replace the nxCxPusher with an AutoEntryScript and be done with it.

... or some other magic that I'm missing. :-)


[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp#1162

	Bob Owen (:bobowen) Assignee
	Comment 20 • 11 years ago

Comment on attachment 8397907 [details] [diff] [review]
Part 2: Replace AutoPushJSContext in nsGlobalWindow::CreateOuterObject v2

(In reply to Bobby Holley (:bholley) from comment #15)

> Nit - I think that IsChromeWindow() should be aligned with the open-paren of
> NewOuterWindowProxy. But does the indentation need to change here at all?

Ah, yes ... I think it was because it was over 80 chars and I'd messed around with the code a bit, before putting it back again.

Anyway, it's all academic now as the function appears to have been nuked. :-)

	Bob Owen (:bobowen) Assignee
	Comment 21 • 11 years ago

Comment on attachment 8398482 [details] [diff] [review]
Part 5: Replace AutoPushJSContext in nsGlobalWindow::SecurityCheckURL v1

This instance of AutoPushJSContext will be dealt with by bug 997067.

	Bob Owen (:bobowen) Assignee
	Comment 22 • 11 years ago

Things have changed quite a bit in the last couple of weeks, so I'm r?ing again just to make sure I'm using things correctly.


Oh ... and I also meant to mention in comment 19, that I noticed a use of an unpushed context had crept in to SetNewDocument where it calls ClearCachedDocumentValue.
I don't think it can ever run though as it's inside an IsInnerWindow(), and we're always outer here I believe.
So maybe that comment should be fixed as well.
I'll get rid of this once we've decided what we need to do for Part 1.
It also makes me think I am right to move the definition of cx from where it is until after we've Auto-whatever-ed.

	Bobby Holley (:bholley)
	Comment 23 • 11 years ago

Comment on attachment 8397893 [details] [diff] [review]
Part 1: Replace nxCxPusher in nsGlobalWindow::SetNewDocument v2

I'm handling this patch over in bug 997440.

	Bobby Holley (:bholley)
	Comment 24 • 11 years ago

Comment on attachment 8407537 [details] [diff] [review]
Part 4: Replace AutoPushJSContext in PostMessageEvent::Run v2

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

::: dom/base/nsGlobalWindow.cpp
@@ +7867,1 @@
>    MOZ_ASSERT(cx);

I think this MOZ_ASSERT can go now.

	Bobby Holley (:bholley)
	Comment 25 • 11 years ago

(In reply to Bob Owen (:bobowen) from comment #22)
> Oh ... and I also meant to mention in comment 19, that I noticed a use of an
> unpushed context had crept in to SetNewDocument where it calls
> ClearCachedDocumentValue.
> I don't think it can ever run though as it's inside an IsInnerWindow(), and
> we're always outer here I believe.

Good catch! My patch in bug 997440 fixes this.

	Bob Owen (:bobowen) Assignee
	Comment 26 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #24)
 
> I think this MOZ_ASSERT can go now.

OK, thanks ... I was just in the middle of cancelling this review with the following :-) ...

So, looking at the try push [1] it seems scriptContext can be null and when it is the constructor for AutoJSAPIWithErrorsReportedToWindow blows up.

So, I guess, when the scriptContext exists we want an AutoJSAPIWithErrorsReportedToWindow, but when it doesn't we want an AutoJSAPI.

I can do this with Maybe<>s like in try push [2], but I can't help thinking there's a more straight forward way of doing it.

I've also got one running with just an AutoJSAPI [3], to see if we can get away with it.

Sorry, to keep pestering you.

[1] https://tbpl.mozilla.org/?tree=Try&rev=0e5c61445868
[2] https://tbpl.mozilla.org/?tree=Try&rev=d345a7596a14
[3] https://tbpl.mozilla.org/?tree=Try&rev=73a84c89d9c1

	Bobby Holley (:bholley)
	Comment 27 • 11 years ago

Yeah, come to think of it, we can probably just use an AutoJSAPI here. I don't think there should be any error messages at this phase (after serializing the message, but before dispatching it) that script would be expecting to hear about. And honestly, it doesn't make a ton of sense to report these things to the target DOMWindow. So maybe an AutoJSAPI is just fine.

	Bob Owen (:bobowen) Assignee
	Comment 28 • 10 years ago

The AutoJSAPI seems to be fine as far as the try push is concerned.
https://tbpl.mozilla.org/?tree=Try&rev=73a84c89d9c1

r?ing again because I think it will be difficult for whoever does the check-in for me to following the thread if I just carry it.

	Bobby Holley (:bholley)
	Comment 29 • 10 years ago

Comment on attachment 8409024 [details] [diff] [review]
Part 4: Replace AutoPushJSContext in PostMessageEvent::Run v3

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

::: dom/base/nsGlobalWindow.cpp
@@ +7861,5 @@
>                      "should have been passed an outer window!");
>  
> +  AutoJSAPI jsapi;
> +  JSContext* cx = jsapi.cx();
> +  JSAutoCompartment ac(cx, mTargetWindow->GetWrapperPreserveColor());

So, this is technically fine, because mTargetWindow is an outer window, and its reflector should always be same-compartment with the current inner (which we select below). But I think it would be clearer to move this JSAutoCompartment underneath the part below where we select targetWindow, and use targetWindow->GetWrapperPreserveColor().

r=me with that.

	Bob Owen (:bobowen) Assignee
	Comment 30 • 10 years ago

r=bholley - from comment 29 with the change to the JSAutoCompartment.

Thanks bholley.

	Bob Owen (:bobowen) Assignee
	Comment 31 • 10 years ago

Landing  order:
https://bugzilla.mozilla.org/attachment.cgi?id=8398446
https://bugzilla.mozilla.org/attachment.cgi?id=8409034
https://bugzilla.mozilla.org/attachment.cgi?id=8398533

Final try push:
https://tbpl.mozilla.org/?tree=Try&rev=232abf7da394

Thanks

	Ryan VanderMeulen [:RyanVM]
	Comment 32 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/967d37e175ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e6a58352cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/d75969295963

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/967d37e175ed
https://hg.mozilla.org/mozilla-central/rev/d0e6a58352cd
https://hg.mozilla.org/mozilla-central/rev/d75969295963