Change usages of AutoPushJSContext to AutoEntryScript for bug 951991 - batch 1

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

Trunk
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments, 22 obsolete attachments)

3.26 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
990 bytes, patch
bholley
: review+
Details | Diff | Splinter Review
1.98 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
3.40 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
2.46 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
2.07 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
3.61 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
11.01 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
3.50 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
This is the first in a series of bugs to change usages of AutoPushJSContext to AutoEntryScript, or something else when this is not required.
See bug 951991 for details.
Hopefully the really easy one first.
I might need to consult about some of the others, but we have family round, so that will be all for tonight.
Attachment #8383819 - Flags: review?(bobbyholley)
Attachment #8383819 - Flags: review?(bobbyholley) → review+
We had discussed removing this and just having a request, because nsJSContext::EvaluateString that gets called shortly afterwards also does a push with AutoCxPusher.
This EvaluateString appears to just be a wrapper around nsJSUtils::EvaulateString, which asserts that the passed context is the current one.
All the other callers to nsJSContext::EvaluateString use AutoPushJSContext, so I think the AutoCxPusher (which appears to push even if the context is already top) could be replaced with a MOZ_ASSERT like in nsJSUtils::EvaluateString.
Attachment #8384619 - Flags: review?(bobbyholley)
Comment on attachment 8384619 [details] [diff] [review]
Part 2: AutoPushJSContext in nsGlobalWindow::RunTimeoutHandler

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

(In reply to Bob Owen (:bobowen) from comment #2)
> We had discussed removing this and just having a request, because
> nsJSContext::EvaluateString that gets called shortly afterwards also does a
> push with AutoCxPusher.
> This EvaluateString appears to just be a wrapper around
> nsJSUtils::EvaulateString, which asserts that the passed context is the
> current one.
> All the other callers to nsJSContext::EvaluateString use AutoPushJSContext,
> so I think the AutoCxPusher (which appears to push even if the context is
> already top) could be replaced with a MOZ_ASSERT like in
> nsJSUtils::EvaluateString.

It seems like we could just short-circuit nsJSContext::EvaluateString and go directly to nsJSUtils::EvaluateString, right? That would definitely move us in the right direction.

This patch looks good conceptually. I think it would be better for our long-term ergonomics to (a) assert that any cx we pass in is identical to the one we would get if we didn't pass one and (b) if AutoEntryScript was castable to a JSContext, so we didn't have to have a second JSContext pointer lying around.

I'll attach a patch that adds those things here. r=me with this patch rebased on top of that one.
Attachment #8384619 - Flags: review?(bobbyholley) → review+
Let me know if this works with your patches. Once it does, we can flag Boris
for review.
Attachment #8384703 - Flags: feedback?(bobowencode)
(In reply to Bobby Holley (:bholley) from comment #3)
> Comment on attachment 8384619 [details] [diff] [review]
> Part 2: AutoPushJSContext in nsGlobalWindow::RunTimeoutHandler
 
> It seems like we could just short-circuit nsJSContext::EvaluateString and go
> directly to nsJSUtils::EvaluateString, right? That would definitely move us
> in the right direction.

I did wonder about that and I suppose with the desire for all the JSContext stuff to go foom, it makes sense.
Shall I do that for all the callers as I come to them and then remove this function on the last one.
They are probably good candidates for me to tackle after the first 5 we talked about.
 
> This patch looks good conceptually. I think it would be better for our
> long-term ergonomics to (a) assert that any cx we pass in is identical to
> the one we would get if we didn't pass one and (b) if AutoEntryScript was
> castable to a JSContext, so we didn't have to have a second JSContext
> pointer lying around.

Yeah, I thought replacing one line with two seemed a little clunky.
r?ed again as I'm not sure, if you wanted me to start to get rid of calls to nsJSContext::EvaluateString.
Also, I started by keeping the JSContext pointer and implicitly casting to it.
But then I thought, as we want to get rid of these you may have meant to keep the AutoEntryScript and pass that instead of the context.
Attachment #8384797 - Flags: review?(bobbyholley)
Attachment #8384619 - Attachment is obsolete: true
Attachment #8384703 - Flags: feedback?(bobowencode) → feedback+
Comment on attachment 8384797 [details] [diff] [review]
Part 2: AutoPushJSContext in nsGlobalWindow::RunTimeoutHandler

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

(In reply to Bob Owen (:bobowen) from comment #5)
> Shall I do that for all the callers as I come to them and then remove this
> function on the last one.
> They are probably good candidates for me to tackle after the first 5 we
> talked about.

Yes please!


(In reply to Bob Owen (:bobowen) from comment #6)
> Created attachment 8384797 [details] [diff] [review]
> Part 2: AutoPushJSContext in nsGlobalWindow::RunTimeoutHandler
> 
> r?ed again as I'm not sure, if you wanted me to start to get rid of calls to
> nsJSContext::EvaluateString.
> Also, I started by keeping the JSContext pointer and implicitly casting to
> it.
> But then I thought, as we want to get rid of these you may have meant to
> keep the AutoEntryScript and pass that instead of the context.

Exactly.

Though I guess this is semantically a little bit weird, since it's not immediately obvious that |entryScript| casts to JSContext*. I'll switch this to a getter. sec
Attachment #8384797 - Flags: review?(bobbyholley) → review+
I was just looking how I might carry forward an r+ using bzexport and found an IRC log where ehsan is suggesting that they shouldn't be.

It doesn't really explain what you should do so I've r?ed again.
Attachment #8384797 - Attachment is obsolete: true
Attachment #8384933 - Flags: review?(bobbyholley)
And again after a qref :-)
Attachment #8384933 - Attachment is obsolete: true
Attachment #8384933 - Flags: review?(bobbyholley)
Attachment #8384940 - Flags: review?(bobbyholley)
Comment on attachment 8384940 [details] [diff] [review]
Part 2: AutoPushJSContext in nsGlobalWindow::RunTimeoutHandler v2

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

To carry over review, I usually do r=reviewer in the commit message and self-r+ the patch on bugzilla.

::: dom/base/nsGlobalWindow.cpp
@@ +11757,5 @@
>      const char* filename = nullptr;
>      uint32_t lineNo = 0;
>      handler->GetLocation(&filename, &lineNo);
>  
> +    AutoEntryScript entryScript(this, NS_IsMainThread(),

Oh, you can just pass |true| for NS_IsMainThread() here. Non-worker DOM stuff only happens on the main thread.
Attachment #8384940 - Flags: review?(bobbyholley) → review+
Comment on attachment 8384863 [details] [diff] [review]
Make AutoEntryScript usable as a JSContext*, and add some assertions. v2

r=me
Attachment #8384863 - Flags: review?(bzbarsky) → review+
r=bholley - Addressed final point from Comment 11.
Attachment #8384940 - Attachment is obsolete: true
Attachment #8385177 - Flags: review+
I created the AutoEntryScript using |global| at first (and I guessed we must be main thread here as well, because AutoEntryScript only creates a request (via AutoCxPusher) when main thread and we need a request here.)
Removed the null check for the JSContext, given that AutoEntryScript now asserts it must have one.
Removed the NS_ASSERTION that looks like a copy paste bug anyway.

However, when I ran the tests for this area I got an assertion from ScriptSettings.h:81 - "No outer windows allowed".
So, reading the description for nsIDcoument::GetInnerWindow it seemed that it might be what we want.
All the tests for this now pass, but I'm just guessing here.
Attachment #8385303 - Flags: review?(bobbyholley)
Just a straight switch to AutoJSContext here.
Attachment #8385405 - Flags: review?(bobbyholley)
Attachment #8385405 - Flags: review?(bobbyholley) → review+
Comment on attachment 8385303 [details] [diff] [review]
Part 3: AutoPushJSContext in nsXULTemplateBuilder::InitHTMLTemplateRoot

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

::: content/xul/templates/src/nsXULTemplateBuilder.cpp
@@ +1383,5 @@
>      if (! context)
>          return NS_ERROR_UNEXPECTED;
>  
> +    nsCOMPtr<nsIGlobalObject> innerGlobal =
> +        do_QueryInterface(doc->GetInnerWindow());

This is exactly right. Kudos!

@@ +1384,5 @@
>          return NS_ERROR_UNEXPECTED;
>  
> +    nsCOMPtr<nsIGlobalObject> innerGlobal =
> +        do_QueryInterface(doc->GetInnerWindow());
> +    AutoEntryScript entryScript(innerGlobal, true, context->GetNativeContext());

Just nix the cx param here, as well as the declaration of nsIScriptContext above. This stuff isn't perf-sensitive, and we should be fine to let AutoEntryScript automatically pull the cx off the global.

@@ +1386,5 @@
> +    nsCOMPtr<nsIGlobalObject> innerGlobal =
> +        do_QueryInterface(doc->GetInnerWindow());
> +    AutoEntryScript entryScript(innerGlobal, true, context->GetNativeContext());
> +
> +    JS::Rooted<JSObject*> scope(entryScript.cx(), global->GetGlobalJSObject());

In cases like this, it'd be fine to just do

JSContext *jscontext = entryScript.cx();

to avoid churning all the lines. Doesn't matter much though.
Attachment #8385303 - Flags: review?(bobbyholley) → review+
r=bholley - comments from previous r+ addressed below.

(In reply to Bobby Holley (:bholley) from comment #16)
> Comment on attachment 8385303 [details] [diff] [review]
> Part 3: AutoPushJSContext in nsXULTemplateBuilder::InitHTMLTemplateRoot

Thanks bholley.

> > +    AutoEntryScript entryScript(innerGlobal, true, context->GetNativeContext());
> 
> Just nix the cx param here, as well as the declaration of nsIScriptContext
> above. This stuff isn't perf-sensitive, and we should be fine to let
> AutoEntryScript automatically pull the cx off the global.

Removed.

> > +    JS::Rooted<JSObject*> scope(entryScript.cx(), global->GetGlobalJSObject());
> 
> In cases like this, it'd be fine to just do
> 
> JSContext *jscontext = entryScript.cx();
> 
> to avoid churning all the lines. Doesn't matter much though.

OK, done.
Wasn't sure how keen we were to stop using JSContext, although I suppose this will all have to be re-visited and tidied up when we do anyway.
Attachment #8385303 - Attachment is obsolete: true
Attachment #8386019 - Flags: review+
(In reply to Bob Owen (:bobowen) from comment #17)
> Wasn't sure how keen we were to stop using JSContext, although I suppose
> this will all have to be re-visited and tidied up when we do anyway.

Exactly. There'll be some churn no matter what - we just want it to be mechanical/uninteresting churn.
Took a while to decide if this was always main thread because of the |aOffThreadToken|, but I think this is to do with something that has happened off thread.
Not entirely sure, the routes into this (via the fuction that calls it) are pretty convoluted.

It sparks another question though ... if we weren't main thread, would we ever need a to be in a JS Request?
Because AutoEntryScript doesn't create one if we are not main thread.
Attachment #8386269 - Flags: review?(bobbyholley)
Comment on attachment 8386269 [details] [diff] [review]
Part 5: AutoPushJSContext in nsScriptLoader::EvaluateScript

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

Yeah, the off-main-thread thing is just a flag telling the JS engine to do the compilation off-main-thread.
Attachment #8386269 - Flags: review?(bobbyholley) → review+
(In reply to Bob Owen (:bobowen) from comment #19)
> Created attachment 8386269 [details] [diff] [review]
> Part 5: AutoPushJSContext in nsScriptLoader::EvaluateScript

Meant to say this is an (A) from bug 951991, as it is in the spec here (Step 2. Sub-step 6.):
http://www.whatwg.org/specs/web-apps/current-work/multipage/scripting-1.html#execute-the-script-block
You can probably land this batch (along with my patch) at this point.
(In reply to Bobby Holley (:bholley) from comment #22)
> You can probably land this batch (along with my patch) at this point.

I was hoping to squeeze in the other callers of nsJSContext::EvaluateScript and chop it.

Here's the first, which I believe is a type (C) from bug 951991.
The way this code used to get the JSContext and nsIScriptContext seemed a bit crazy, but we no longer need the direct reference to the script context now.
Attachment #8386977 - Flags: review?(bobbyholley)
This one seemed fairly straight forward.
It's a type (A) as I think it is step 11 from:
http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#javascript-protocol
which eventually leads to the jump to a code entry-point.
Attachment #8386985 - Flags: review?(bobbyholley)
Bit doubtful of this one, as I've grabbed and passed through the inner global.
Otherwise, I think I'd have had to go through the whole rigmarole again to get it in InstallField.

Also, I noticed while I was getting rid of nsJSContext::EvaluateString that there was an |NS_ENSURE_TRUE(mIsInitialized, NS_ERROR_NOT_INITIALIZED);| at the top of it.
Is this something I need to worry about?
Attachment #8386991 - Flags: review?(bobbyholley)
The build for this was failing on B2G as I'd not included mozilla/dom/ScriptSettings.h
Not sure why it works for the others.

I notice in nsGlobalWindow.cpp that it is included with and without the mozilla/dom/

I also noticed that mozilla/dom/Element.h was included twice.
Attachment #8387000 - Flags: review?(bobbyholley)
Attachment #8386269 - Attachment is obsolete: true
Just noticed that I could remove the inclusion of nsCxPusher.h

r=bholley - from comment 16 (with issues addressed in comment 17).
Attachment #8386019 - Attachment is obsolete: true
Attachment #8387004 - Flags: review+
Comment on attachment 8386977 [details] [diff] [review]
Part 6: AutoPushJSContext in nsNPAPIPlugin.cpp _evaluate

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

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +1488,5 @@
>  
> +  nsCOMPtr<nsIGlobalObject> innerGlobal =
> +    do_QueryInterface(doc->GetInnerWindow());
> +  mozilla::dom::AutoEntryScript entryScript(innerGlobal, true,
> +                                            GetJSContextFromDoc(doc));

Why do we need an entryScript here? Seems like an AutoSafeJSContext would be fine.
Comment on attachment 8386977 [details] [diff] [review]
Part 6: AutoPushJSContext in nsNPAPIPlugin.cpp _evaluate

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

(In particular, this is really just plumbing, and never runs script that would expect any particular entry point).
Attachment #8386977 - Flags: review?(bobbyholley)
Comment on attachment 8387000 [details] [diff] [review]
Part 5: AutoPushJSContext in nsScriptLoader::EvaluateScript

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

Feel free to mess around with #includes without requesting re-review. :-)
Attachment #8387000 - Flags: review?(bobbyholley) → review+
Comment on attachment 8386985 [details] [diff] [review]
Part 7: AutoPushJSContext in nsJSThunk::EvaluateScript

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

Looks great.

One thing that I think we should start doing is including a comment above each AutoEntryPoint explaining why it's there. If it corresponds to a spec concept, we should link to the relevant part of the spec. If not, we should explain exactly why we need it. Can you add such comments to all your patches?

r=me with that
Attachment #8386985 - Flags: review?(bobbyholley) → review+
Comment on attachment 8386991 [details] [diff] [review]
Part 8: AutoPushJSContext in nsXBLProtoImplField::InstallField* * *

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

(In reply to Bob Owen (:bobowen) from comment #25)
> Bit doubtful of this one, as I've grabbed and passed through the inner
> global.
> Otherwise, I think I'd have had to go through the whole rigmarole again to
> get it in InstallField.


> Also, I noticed while I was getting rid of nsJSContext::EvaluateString that
> there was an |NS_ENSURE_TRUE(mIsInitialized, NS_ERROR_NOT_INITIALIZED);| at
> the top of it.
> Is this something I need to worry about?

Nah.

::: dom/xbl/nsXBLProtoImplField.cpp
@@ +396,5 @@
>    return NS_OK;
>  }
>  
>  nsresult
>  nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,

We don't need |aContext| anymore, right?

@@ +399,5 @@
>  nsresult
>  nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,
>                                    JS::Handle<JSObject*> aBoundNode,
>                                    nsIURI* aBindingDocURI,
> +                                  nsIGlobalObject* aInnerGlobal,

Instead of this, I think it would make more sense just to pass thisObj to InstallField, and extract the global there. And we can probably shortcut the whole xblNode bit, by just doing xpc::WindowGlobalOrNull(js::GetGlobalForObjectCrossCompartment(thisObj));

Make sense?
Attachment #8386991 - Flags: review?(bobbyholley)
r=bholley - from comment 11

Just added a code comment to link to the relevant part of the HTML spec.
I've not been too specific (e.g. with step numbers) in case of changes to the spec.
Attachment #8387472 - Flags: review?(bobowencode)
Attachment #8385177 - Attachment is obsolete: true
Comment on attachment 8387472 [details] [diff] [review]
Part 2: AutoPushJSContext in nsGlobalWindow::RunTimeoutHandler

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

r=bholley - from comment 11

Doesn't look like you can carry reviews with bzexport.
It would be really useful if you could.
Attachment #8387472 - Flags: review?(bobowencode) → review+
r=bholley - from comment 16 (with issues addressed in comment 17).

Added code comment to explain the use of AutoEntryScript.
Attachment #8387004 - Attachment is obsolete: true
Attachment #8387503 - Flags: review+
(In reply to Bobby Holley (:bholley) from comment #29)
> Comment on attachment 8386977 [details] [diff] [review]
> Part 6: AutoPushJSContext in nsNPAPIPlugin.cpp _evaluate
> 
> Review of attachment 8386977 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In particular, this is really just plumbing, and never runs script that
> would expect any particular entry point).

I tried just replacing with an AutoSafeJSContext, but I got 19 failures in 
dom/plugins/test/mochitest/test_npruntime_npnevaluate.html

Various things like:
failed | npnEvaluateTest returned an unexpected value - got undefined, expected 3
Flags: needinfo?(bobbyholley)
r=bholley - from comment 31

Added a code comment to link to the relevant part of the HTML spec.
Attachment #8386985 - Attachment is obsolete: true
Attachment #8387546 - Flags: review+
(In reply to Bobby Holley (:bholley) from comment #32)
> Comment on attachment 8386991 [details] [diff] [review]
> Part 8: AutoPushJSContext in nsXBLProtoImplField::InstallField* * *
> 
> Review of attachment 8386991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
 
> >  nsresult
> >  nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,
> 
> We don't need |aContext| anymore, right?

Well, I'm using it to get the JSContext.
Do you mean, don't pass a JSContext and allow AutoEntryScript to get it from the global.

> @@ +399,5 @@
> >  nsresult
> >  nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,
> >                                    JS::Handle<JSObject*> aBoundNode,
> >                                    nsIURI* aBindingDocURI,
> > +                                  nsIGlobalObject* aInnerGlobal,
> 
> Instead of this, I think it would make more sense just to pass thisObj to
> InstallField, and extract the global there. And we can probably shortcut the
> whole xblNode bit, by just doing
> xpc::WindowGlobalOrNull(js::GetGlobalForObjectCrossCompartment(thisObj));
> 
> Make sense?

Do you mean get rid of all of the xblNode and global stuff from InstallXBLField (even though there are checks that return near the top) ...
or
Just re-get the global in InstallField?

I assume |xpc::WindowGlobalOrNull(js::GetGlobalForObjectCrossCompartment(thisObj));| returns the inner global?

Also, I notice that xpc::WindowGlobalOrNull calls GetGlobalForObjectCrossCompartment again, why do we have to call it twice?

Sorry about all the questions.
(In reply to Bob Owen (:bobowen) from comment #36)
> (In reply to Bobby Holley (:bholley) from comment #29)
> > Comment on attachment 8386977 [details] [diff] [review]
> > Part 6: AutoPushJSContext in nsNPAPIPlugin.cpp _evaluate
> > 
> > Review of attachment 8386977 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > (In particular, this is really just plumbing, and never runs script that
> > would expect any particular entry point).
> 
> I tried just replacing with an AutoSafeJSContext, but I got 19 failures in 
> dom/plugins/test/mochitest/test_npruntime_npnevaluate.html

Did you also make sure to enter the document's compartment?


(In reply to Bob Owen (:bobowen) from comment #38)
> Well, I'm using it to get the JSContext.
> Do you mean, don't pass a JSContext and allow AutoEntryScript to get it from
> the global.

Yeah. And more specifically, do the AutoEntryScript itself in InstallField.

> 
> > @@ +399,5 @@
> > >  nsresult
> > >  nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,
> > >                                    JS::Handle<JSObject*> aBoundNode,
> > >                                    nsIURI* aBindingDocURI,
> > > +                                  nsIGlobalObject* aInnerGlobal,
> > 
> > Instead of this, I think it would make more sense just to pass thisObj to
> > InstallField, and extract the global there. And we can probably shortcut the
> > whole xblNode bit, by just doing
> > xpc::WindowGlobalOrNull(js::GetGlobalForObjectCrossCompartment(thisObj));
> > 
> > Make sense?
> 
> Do you mean get rid of all of the xblNode and global stuff from
> InstallXBLField (even though there are checks that return near the top)

Which? Just the null check? From what I can tell most of this stuff can just be collapsed and simplifieid into InstallField.

> I assume
> |xpc::WindowGlobalOrNull(js::GetGlobalForObjectCrossCompartment(thisObj));|
> returns the inner global?

Yes. There's no such thing as an 'outer global'. There are Outer Windows, whose return value from GetGlobalJSObject() is not actually a global at all.

> Also, I notice that xpc::WindowGlobalOrNull calls
> GetGlobalForObjectCrossCompartment again, why do we have to call it twice?

Oh, looks like you don't need to then.
 
> Sorry about all the questions.

No problem!
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #39)

> > I tried just replacing with an AutoSafeJSContext, but I got 19 failures in 
> > dom/plugins/test/mochitest/test_npruntime_npnevaluate.html
> 
> Did you also make sure to enter the document's compartment?

Err no, sorry haven't quite got my head round all of this, I'll try that.

> (In reply to Bob Owen (:bobowen) from comment #38)
> > Well, I'm using it to get the JSContext.
> > Do you mean, don't pass a JSContext and allow AutoEntryScript to get it from
> > the global.
> 
> Yeah. And more specifically, do the AutoEntryScript itself in InstallField.

OK.
AutoEntryScript was already in InstallField, I suppose it could be done in InstallXBLField in which case I wouldn't need to worry about the global in IntsallField.

> > Do you mean get rid of all of the xblNode and global stuff from
> > InstallXBLField (even though there are checks that return near the top)
> 
> Which? Just the null check? From what I can tell most of this stuff can just
> be collapsed and simplifieid into InstallField.

The null check on native and on xblNode, one returns true the other throws and returns false.
(In reply to Bob Owen (:bobowen) from comment #40)

> AutoEntryScript was already in InstallField, I suppose it could be done in
> InstallXBLField in which case I wouldn't need to worry about the global in
> IntsallField.

I'd probably put it in the innermost call, and just collapse all the script munging there.
 
> > > Do you mean get rid of all of the xblNode and global stuff from
> > > InstallXBLField (even though there are checks that return near the top)
> > 
> > Which? Just the null check? From what I can tell most of this stuff can just
> > be collapsed and simplifieid into InstallField.
> 
> The null check on native and on xblNode, one returns true the other throws
> and returns false.

Those can move into the inner function as well, right?
OK, I think I understand now.
The AutoPushJSContext pushes the document's context with an AutoCxPusher which automatically enters the compartment of the document by getting the window proxy from the script context of the JSContext.

However the AutoSafeJSContext just enters the compartment of the SafeJSContextGlobal, so we need to enter the compartment of the document ourselves, which we can do in a more direct way.
Attachment #8388084 - Flags: review?(bobbyholley)
Attachment #8386977 - Attachment is obsolete: true
Comment on attachment 8388084 [details] [diff] [review]
Part 6: AutoPushJSContext in nsNPAPIPlugin.cpp _evaluate

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

r=me with that

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +1484,5 @@
>  
>    nsIDocument *doc = GetDocumentFromNPP(npp);
>    NS_ENSURE_TRUE(doc, false);
>  
> +  nsCOMPtr<nsIGlobalObject> innerGlobal =

Nit - The term 'innerGlobal' is a bit of a misnomer. It should either be 'global' or 'innerWin'.

Note that nsPIDOMWindow, which is the return value from GetInnerWindow, is static_castable to nsGlobalWindow*, so you can skip the QI (though you still need a null check for both the C++ object and the reflector). So:

nsGlobalWindow* win = static_cast<nsGlobalWindow*>(doc->GetInnerWindow());
if (NS_WARN_IF(!win || win->FastGetGlobalJSObject())) {
  return false;
}
AutoSafeJSContext cx;
JSAutoCompartment ac(cx, win->FastGetGlobalJSObject());
Attachment #8388084 - Flags: review?(bobbyholley) → review+
r=bholley - from comment 43.

Final review changes applied.
Attachment #8388084 - Attachment is obsolete: true
Attachment #8388218 - Flags: review+
Well I'm pretty sure I'm being rather dim here, but I think I've done what you were suggesting.
The tests still seem to pass OK.

So, I've used WindowGlobalOrNull to get the nsIGlobalObject for the AutoEntryScript.
Then I've moved the checks from InstallXBLField after that, as I need the JSContext for that code.

I think that you were implying that these checks could be simplified, although I can't quite see how given that the behaviour is different in the two cases.

I think that you're tied up with other things at the moment, so I'll wait to ask for the review when I know you're free.
Attachment #8386991 - Attachment is obsolete: true
Just spotted that I'd left the forward declaration of nsIGlobalObject in nsXMLProtoImplField.h from my last attempt.

See comment 45 for other ramblings.
Attachment #8388223 - Attachment is obsolete: true
Comment on attachment 8388223 [details] [diff] [review]
Part 8: AutoPushJSContext in nsXBLProtoImplField::InstallField

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

No need to wait to flag for review. I can always ignore the request if I don't have time. ;-)
Attachment #8388223 - Flags: review?(bobbyholley)
Attachment #8388223 - Flags: review?(bobbyholley)
Attachment #8388232 - Flags: review?(bobbyholley)
Comment on attachment 8388232 [details] [diff] [review]
Part 8: AutoPushJSContext in nsXBLProtoImplField::InstallField

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

Looks good modulo the bit below.

::: dom/xbl/nsXBLProtoImplField.cpp
@@ -160,5 @@
> -    // here.
> -    //
> -    // We could make this stricter by checking the class maybe, but whatever.
> -    return true;
> -  }

Sorry for not looking at this more closely before. I think this part of the check can just go away. The reason is that we only reach InstallXBLField if ValueHasISupportsPrivate is true (and we even assert so above).

@@ -166,5 @@
> -  nsCOMPtr<nsIContent> xblNode = do_QueryInterface(native);
> -  if (!xblNode) {
> -    xpc::Throw(cx, NS_ERROR_UNEXPECTED);
> -    return false;
> -  }

As for this part of the check, I think we can just roll it into the machinery that is currently done in ValueHasISupportsPrivate. Given that XBL bindings only apply to Elements, and Elements are all on the new DOM bindings, ValueHasISupportsPrivate can just be replaced by:

bool
ValueIsElement(JS::Handle<JS::Value> v) {
  if (!v.isObject()) {
    return false;
  }
  Element *e;
  UNWRAP_OBJECT(Element, &v.toObject(), e);
  return !!e;
}
Attachment #8388232 - Flags: review?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #49)
> Comment on attachment 8388232 [details] [diff] [review]
> Looks good modulo the bit below.

Thanks.

Spent yesterday in the wonderful world of Windows and sandboxing.

> Sorry for not looking at this more closely before. I think this part of the
> check can just go away. The reason is that we only reach InstallXBLField if
> ValueHasISupportsPrivate is true (and we even assert so above).

> ... ValueHasISupportsPrivate can just be replaced by:
> 
> bool
> ValueIsElement(JS::Handle<JS::Value> v) {
>   if (!v.isObject()) {
>     return false;
>   }
>   Element *e;
>   UNWRAP_OBJECT(Element, &v.toObject(), e);
>   return !!e;
> }

ValueHasISupportsPrivate seems to be doing something slightly different.

If GetDOMClass returns null then it checks flags on the return of JS_GetClass.

Whereas UnwrapObject, if GetDOMClass initially returns null, does a js::CheckedUnwrap and calls GetDOMClass again on the return value.
The upshot is that sometimes ValueIsElement returns true when ValueHasISupportsPrivate doesn't and some tests fail.
At least one of these is with a <field> with a readonly attribute.

Also, the extra check of:
  nsISupports* native =
    nsContentUtils::XPConnect()->GetNativeOfWrapper(cx, thisObj);
  if (!native) {

in InstallXBLField, seems to be needed.
If I remove that (even with keeping ValueHasISupportsPrivate) 2 other tests fail.
These seem to be checking that fields aren't on the proto.
Seems like GetNativeOfWrapper drops down to UnwrapDOMObjectToISupports which returns null in some cases.

I can remove the xblNode null check without affecting any of the tests.

I'll carry on trying to work out what's going on, but I thought I'd let you know where I'm up to.
> If GetDOMClass returns null then it checks flags on the return of JS_GetClass.

Right, but in what cases is that flag set?

> The upshot is that sometimes ValueIsElement returns true when ValueHasISupportsPrivate
> doesn't

Hrm.  Are those cases in which "this" is a cross-compartment wrapper for an element?  What tests are relying on us not having working fields in that case?
(In reply to Boris Zbarsky [:bz] from comment #51)
> > If GetDOMClass returns null then it checks flags on the return of JS_GetClass.
> 
> Right, but in what cases is that flag set?

Seems like most of the time in ValueHasISupportsPrivate it returns at:
|    return domClass->mDOMObjectIsISupports;|

But when it doesn't, it mainly (possibly always, at least in the tests I'm running) seems to return false and |clasp->name| is "Proxy".
Is this the window proxy?

I've got both sets of code in the method and at the same time |!!e| is true;

So with ValueIsElement, could things be getting set in the window and then not where they should be?
I'll try and see, but it may not be tonight.

> > The upshot is that sometimes ValueIsElement returns true when ValueHasISupportsPrivate
> > doesn't
> 
> Hrm.  Are those cases in which "this" is a cross-compartment wrapper for an
> element?  What tests are relying on us not having working fields in that
> case?

I'm not sure, I think most of the tests are accessing elements that are all in the same xhtml file.
Although to be honest this is the first time I've come across any of the XBL stuff, so I'm flying fairly blind.

All of the tests in this file fail with ValueIsElement:
http://dxr.mozilla.org/mozilla-central/source/dom/xbl/test/test_bug542406.xhtml
> Is this the window proxy?

Could be any proxy.  The way to tell is to examine the concrete type of js::GetProxyHandler(&v.toObject()).

> All of the tests in this file fail with ValueIsElement:

Huh.  That's odd.  I mean, ValueHasISupportsPrivate() returning _true_ should make fields work when they shouldn't or something, right?  But in this case it makes them not work when they should?
FWIW Bob, you're welcome to punt on shaving this yak if you'd rather get on with stuff. But I'm happy to help dig in and fix this up too. Up to you. ;-)
Got back to this, this morning armed with a freshly sharpened yak razor (gdb and a fresh pot of coffee).

So, I think I understand a little more what is happening in dom/xbl/test/test_bug542406.xhtml.

In nsXBLProtoImplField FieldGetter calls:
|JS::CallNonGenericMethod<ValueHasISupportsPrivate, FieldGetterImpl>(cx, args);|

This first calls our Test (ValueHasISupportsPrivate) with args.thisv(), which at this point is a cross compartment wrapper to our HTMLParagraphElement.
This is not a DOMClass and does not have the HAS_PRIVATE_NSISUPPORTS flags and so returns false.

So, JS::CallNonGenericMethod then calls (skipping the chain of calls) CrossCompartmentWrapper::nativeCall which seems to unwrap the arguments (or perhaps re-wraps them in the correct compartment) and re-calls CallNonGenericMethod.
This now calls our Test again and this time thisv is the paragraph element and so is a DOMCLass.

If I put ValueIsElement in, when it gets called with the proxy UNWRAP_OBJECT realises it is a wrapper and unwraps it, finds that it it is an Element so we return true.
So InstallXBLField gets called with the wrapper and because the test has passed CallNonGenericMethod doesn't call CrossCompartmentWrapper::nativeCall.

In the old code (with ValueIsElement) this sails through the checks and retrieval of |global| and |context|, because the first check calls GetNativeOfWrapper, which unwraps the element.
It then promptly blows up in InstallField when it calls xpc::GetXBLScope.

In the new code the first time we interact with |thisObj| is as |aBoundNode| in InstallField -  |xpc::WindowGlobalOrNull(aBoundNode);|
This returns null, and in an attempt to replicate previous behaviour (from InstallXBLField), I return NS_OK and nothing gets installed, so we get undefined. 


So, onto the second yak - why do we need the lines (originally in InstallXBLField):
  nsISupports* native = nsContentUtils::XPConnect()->GetNativeOfWrapper(cx, thisObj);
  if (!native) {
    // Looks like whatever |thisObj| is it's not our nsIContent.  It might well
    // be the proto our binding installed, however, where the private is the
    // nsXBLDocumentInfo, so just baul out quietly.  Do NOT throw an exception
    // here.
    //
    // We could make this stricter by checking the class maybe, but whatever.
    return true;
  }

This seems to be to prevent the field getting set on the __proto__
In dom/xbl/test/test_bug372769.html for the test:
  is(d.__proto__.seventeen, undefined, "Shouldn't have this on proto");

FieldGetter gets called with the prototype, which doesn't seem to be a DOMClass, but passes the subsequent flag check in ValueHasISupportsPrivate.
It then gets caught by the "native" check above and fails silently, which results in undefined.

At first I couldn't understand how these tests were working with ValueIsElement, as the __proto__ isn't an Element.
Eventually I spotted that it wasn't initalising |e| to null and UNWRAP_OBJECT doesn't affect the |value| when it fails, so ValueIsElement was always returning true.
Once, I'd fixed that, it broke the tests completely as the getter throws an exception, so as it is currently structured we need to have the flag checks as well as the native check.


As a slight aside the "seventeen" test above implies that if the field was defined in the binding that we are extending then it should be on the __proto__.
I couldn't see how this would work from the code and sure enough if I add a test of:
  is(d.__proto__.thirteen, "13 ancestor", "Should be on proto");
  
it fails, which seems a bit wrong.


So it might be best to leave ValueHasISupportsPrivate and the |native| and |xblNode| checks in InstallXBLField and just remove the |global| and |context| from there and use WindowGlobalOrNull to get the nsIGlobalObject we want in InstallField.
Not sure how much of an overhead that adds.
I'm also not sure if the current behaviour is correct, but this should replicate it.
> As a slight aside the "seventeen" test above implies that if the field was defined in the
> binding that we are extending then it should be on the __proto__.

The field getter/setter are on the proto.  But they should not actually change the proto in any way.  |is(d.__proto__.thirteen, "13 ancestor", "Should be on proto");| should fail.

It sounds like the basic issue is that the predicate passed to CallNonGenericMethod needs to only return true if the object is really the sort of object we want to work with; it should never unwrap proxies...
(In reply to Boris Zbarsky [:bz] from comment #56)
> The field getter/setter are on the proto.  But they should not actually
> change the proto in any way.  |is(d.__proto__.thirteen, "13 ancestor",
> "Should be on proto");| should fail.

OK, thanks bz.

> It sounds like the basic issue is that the predicate passed to
> CallNonGenericMethod needs to only return true if the object is really the
> sort of object we want to work with; it should never unwrap proxies...

Right, but it also needs to return true for objects on which we don't actually want to install a field, but don't want to throw an exception either.

But, can we (or do we need to) improve much on the checks that are already there?
Also, does using WindowGlobalOrNull in InstallField, but leaving the |native| and |xblNode| checks in InstallXBLField (even though we don't use them for anything else anymore), add much of an overhead?
> But, can we (or do we need to) improve much on the checks that are already there?

I'll claim that's way out of the scope of this bug.
(In reply to Boris Zbarsky [:bz] from comment #58)
> > But, can we (or do we need to) improve much on the checks that are already there?
> 
> I'll claim that's way out of the scope of this bug.

OK thanks.

I've done what I suggested at the bottom of comment 55 and we'll see if bholley thinks I've left the yak looking respectable after my attempts at barbery. :-)
Attachment #8391296 - Flags: review?(bobbyholley)
Attachment #8388232 - Attachment is obsolete: true
Comment on attachment 8391296 [details] [diff] [review]
Part 8: AutoPushJSContext in nsXBLProtoImplField::InstallField

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

(In reply to Bob Owen (:bobowen) from comment #59)
> I've done what I suggested at the bottom of comment 55 and we'll see if
> bholley thinks I've left the yak looking respectable after my attempts at
> barbery. :-)


A and B the C of D!
Attachment #8391296 - Flags: review?(bobbyholley) → review+
r=bholley - from history item 2

nsEventListenerManager has been renamed to EventListenerManager.

I noticed that there are still some references to nsEventListenerManager:
http://dxr.mozilla.org/mozilla-central/search?q=nsEventListenerManager&case=false

Two of them are just text but the others look like they might cause someone some pain.

Do you think they should be fixed up?
What's the protocol here - I assume raising a new bug, that blocks the rename one, is preferable to re-opening the old bug?
Attachment #8383819 - Attachment is obsolete: true
Attachment #8393525 - Flags: review+
Flags: needinfo?(bobbyholley)
r=bholley - from comment 30

Just adding a comment to link the use of the script entry point to the spec.
Attachment #8387000 - Attachment is obsolete: true
Attachment #8393529 - Flags: review+
r=bholley - from comment 60

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

> A and B the C of D!

Thanks ... and that reminded me that I hadn't added a comment for the use of AutoEntryScript, I think this one is a C.

Also went back and checked the others.
I'd missed one on Part 5 - could have sworn I'd already checked them, but the evidence is not on my side.
Attachment #8391296 - Attachment is obsolete: true
Attachment #8393533 - Flags: review+
Here's a try run I prepared earlier:
https://tbpl.mozilla.org/?tree=Try&rev=c72c3b5eafd0

and another quick one to make sure I haven't messed things up since:
https://tbpl.mozilla.org/?tree=Try&rev=242345c71285
(In reply to Bob Owen (:bobowen) from comment #61)
> I noticed that there are still some references to nsEventListenerManager:
> http://dxr.mozilla.org/mozilla-central/
> search?q=nsEventListenerManager&case=false
> 
> Two of them are just text but the others look like they might cause someone
> some pain.
> 
> Do you think they should be fixed up?
> What's the protocol here - I assume raising a new bug, that blocks the
> rename one, is preferable to re-opening the old bug?

Eh, I wouldn't worry about it. trace-malloc/types.dat is full of references to things that have been renamed or removed. I don't know if anyone is actually using that file anymore. And the comments, well, they could be fixed up, but it's not that hard to find the rename. Just keep walking. ;-)
Flags: needinfo?(bobbyholley)
r=bholley - from comment 16 (with issues addressed in comment 17).

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

> and another quick one to make sure I haven't messed things up since:
> https://tbpl.mozilla.org/?tree=Try&rev=242345c71285

Good job I did, Part 3 was failing to compile because it couldn't find AutoEntryScript.
This was definitely compiling before, anyway I've added in the include.

Another try push:
https://tbpl.mozilla.org/?tree=Try&rev=9e9e7d7f1ec4
Attachment #8387503 - Attachment is obsolete: true
Attachment #8393662 - Flags: review+
r=bholley - from comment 16 (with issues addressed in comment 17).

And now I've just spotted that I've used the variable name |innerGlobal| in this patch as well, which bholley said was a poor choice back in comment 43 for Part 6.
Sorry for the churn.
Attachment #8393662 - Attachment is obsolete: true
Attachment #8393700 - Flags: review+
r=bholley - from comment 16 (with issues addressed in comment 17).

Damn, I didn't see that we already had a |global| variable, so my new name is just confusing.
Renamed to innerWin.

We apologise for the inconvenience
Attachment #8393700 - Attachment is obsolete: true
Attachment #8393724 - Flags: review+
And now we're green, so I'm going to try and land these.
Keywords: checkin-needed
So Part 1, bholley's patch, parts 2-8? (FWIW, I shouldn't need to pull up the Try push to figure out the landing order on a checkin-needed request)
Flags: needinfo?(bobowencode)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #70)
> So Part 1, bholley's patch, parts 2-8? (FWIW, I shouldn't need to pull up
> the Try push to figure out the landing order on a checkin-needed request)

Yeah, sorry.
That order would work (it happened to be how it was in my queue), but probably makes most sense to push bholley's patch first and then 1 to 8.

Apologies again, I was watching the try run from my phone and then posted the checkin-needed.
I'd forgotten that the order wasn't at clear.
Flags: needinfo?(bobowencode)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.