Closed Bug 1046647 Opened 10 years ago Closed 10 years ago

Change usages of nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI and derivatives for bug 951991 - batch 13

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(6 files, 4 obsolete files)

1.40 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.67 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.76 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
7.78 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.10 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.86 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Another batch of patches moving nsCxPusher, AutoCxPusher and AutoPushJSContext to AutoJSAPI etc.
See bug 951991 for details.
Doesn't look like the ExecuteScript that takes an nsIScriptContext is used.
It's protected and the class is MOZ_FINAL, so I think we can get rid of it.

This moves that code into the other ExecuteScript, which makes the AutoEntryScript easier to set up.
Attachment #8467114 - Flags: review?(bobbyholley)
As I say in the comment, I think this will need an AutoEntryScript, because |propertyHolder| might be an existing object.

I tried using just an nsIGlobalObject* and letting it get the JSContext* for itself, but I was getting a leak of an AsyncLatencyLogger in crashtests and reftests on Windows.
I assume that this was because sometimes it was ending up with the SafeJSContext, when it wouldn't in the old code.

Couldn't reproduce locally, so I had to go with this.
Attachment #8467117 - Flags: review?(bobbyholley)
I think that this ends up (after a couple of scary layers) in sXPCWrappedJSClass::CallMethod (and at least the only test I could find that triggered this does), which creates an AutoEntryScript anyway.
So, I don't think we need the pusher at all here.

If there are other routes through the code that this could take then this assumption is wrong.
Attachment #8467121 - Flags: review?(bobbyholley)
Comment on attachment 8467114 [details] [diff] [review]
Part 1: Replace AutoCxPusher in XULDocument::ExecuteScript v1

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

::: content/xul/document/src/XULDocument.cpp
@@ +3703,5 @@
> +    // We're about to run script via JS::CloneAndExecuteScript, so we need an
> +    // AutoEntryScript. This is Gecko specific and not in any spec.
> +    AutoEntryScript aes(mScriptGlobalObject);
> +    JSContext* cx = aes.cx();
> +    JS::Rooted<JSObject*> baseGlobal(cx, mScriptGlobalObject->GetGlobalJSObject());

Again, I'd prefer CurrentGlobalOrNull in these situations, because I think it gives us more flexibility in tweaking the AutoJSAPI/nsIGlobalObject setup, and because GetGlobalJSObject() is technically fallible, whereas CurrentGlobalOrNull is not.

@@ +3704,5 @@
> +    // AutoEntryScript. This is Gecko specific and not in any spec.
> +    AutoEntryScript aes(mScriptGlobalObject);
> +    JSContext* cx = aes.cx();
> +    JS::Rooted<JSObject*> baseGlobal(cx, mScriptGlobalObject->GetGlobalJSObject());
> +    NS_ENSURE_TRUE(baseGlobal, NS_ERROR_FAILURE);

And with that, we don't need this. AutoEntryScript currently just asserts that GetGlobalJSObject() doesn't return null.
Attachment #8467114 - Flags: review?(bobbyholley) → review+
I'm mainly going the comment here, over this potentially running script.

I can't see anything in the HTML spec for structured clone about this, so I think it must be Gecko specific.

As a side note, I think either aCx was never null or InitFromJSVal never failed or this used to crash.
Attachment #8467139 - Flags: review?(bobbyholley)
I tried removing this, but it fails the test that Olli originally put in for this bug.
\o/ for tests.

So, I've gone with InitWithLegacyErrorReporting as I'm not entirely sure how this guards the link clicking and it should mean the same context is pushed.
Attachment #8467145 - Flags: review?(bobbyholley)
Comment on attachment 8467117 [details] [diff] [review]
Part 2: Replace AutoCxPusher in nsXBLProtoImpl::InstallImplementation v1

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

XBL installation runs at crazy times (like during layout), so I think we basically never want to run script there (and if we ever did in fact trigger a setter, I would much prefer us to throw than to run it). Can you switch to an AutoJSAPI and describe as much in the comment?
Attachment #8467117 - Flags: review?(bobbyholley) → review-
I would have expected that whatever calls this should have dealt with the context pushing, particularly as it gets used in |initializer->Initialize| without being pushed here.

However as I'm typing this, I'm suddenly less confident that whatever ultimately calls this would have known to create an AutoEntryScript. :/
Attachment #8467154 - Flags: review?(bobbyholley)
Attachment #8467121 - Flags: review?(bobbyholley) → review+
Comment on attachment 8467139 [details] [diff] [review]
Part 4: Replace nsCxPusher in nsDocShell::AddState v1

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

::: content/base/public/nsContentUtils.h
@@ -174,5 @@
>  
> -  /**
> -   * Get a JSContext from the document's scope object.
> -   */
> -  static JSContext* GetContextFromDocument(nsIDocument *aDocument);

So glad to see this go!

::: docshell/base/nsDocShell.cpp
@@ +10752,5 @@
>          scContainer = new nsStructuredCloneContainer();
> +        if (aCx) {
> +            rv = scContainer->InitFromJSVal(aData, aCx);
> +        } else {
> +            // InitFromJSVal might cause script to run, so we need an

How? The structured clone algorithm isn't supposed to run script. I think we want an AutoJSAPI here.
Attachment #8467139 - Flags: review?(bobbyholley) → review+
Comment on attachment 8467139 [details] [diff] [review]
Part 4: Replace nsCxPusher in nsDocShell::AddState v1

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

Er, I meant to rminus this. But after doing some reading, it looks like the structured clone algorithm _can_ in fact run script. I stand corrected.

This is really error prone though with the current InitWithJSVal API though, because it means that the caller has to be sure to use an AutoEntryScript. So I think we should change the InitWithJSVal API (which doesn't have a lot of consumers anyway) to stop taking a cx. If the jsval is a primitive, we just use an AutoJSAPI. If it's an object, we use an AutoEntryScript for that object's global.

Does that seem reasonable?
Attachment #8467139 - Flags: review+ → review-
Comment on attachment 8467145 [details] [diff] [review]
Part 5: Replace nsCxPusher in nsDocShell  OnLinkClickEvent::Run v1

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

(In reply to Bob Owen (:bobowen) from comment #7)
> Created attachment 8467145 [details] [diff] [review]
> Part 5: Replace nsCxPusher in nsDocShell  OnLinkClickEvent::Run v1
> 
> I tried removing this, but it fails the test that Olli originally put in for
> this bug.
> \o/ for tests.
> 
> So, I've gone with InitWithLegacyErrorReporting as I'm not entirely sure how
> this guards the link clicking and it should mean the same context is pushed.

Hm - which tests are these? Are they related to error reporting, or is the goal just to keep the same behavior here? If it's the latter, I think we need to figure out what's actually going on, because otherwise this will all just blow up again as soon as we turn off cx pushing.
Comment on attachment 8467154 [details] [diff] [review]
Part 6: Replace nsCxPusher in nsDomClassInfo BaseStubConstructor v1

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

This gets invoked from script only, so I'm not worried about the caller not having an AutoEntryScript. Glad to see this cx push go.
Attachment #8467154 - Flags: review?(bobbyholley) → review+
Comment on attachment 8467145 [details] [diff] [review]
Part 5: Replace nsCxPusher in nsDocShell  OnLinkClickEvent::Run v1

Clearing review here while waiting for info from Bob.
Attachment #8467145 - Flags: review?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #13)
> Comment on attachment 8467145 [details] [diff] [review]
> Part 5: Replace nsCxPusher in nsDocShell  OnLinkClickEvent::Run v1
> 
> Review of attachment 8467145 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Bob Owen (:bobowen) from comment #7)
> > Created attachment 8467145 [details] [diff] [review]
> > Part 5: Replace nsCxPusher in nsDocShell  OnLinkClickEvent::Run v1
> > 
> > I tried removing this, but it fails the test that Olli originally put in for
> > this bug.
> > \o/ for tests.
> > 
> > So, I've gone with InitWithLegacyErrorReporting as I'm not entirely sure how
> > this guards the link clicking and it should mean the same context is pushed.
> 
> Hm - which tests are these? Are they related to error reporting, or is the
> goal just to keep the same behavior here? If it's the latter, I think we
> need to figure out what's actually going on, because otherwise this will all
> just blow up again as soon as we turn off cx pushing.

Test was content/base/test/test_bug666604.html, I'll look at this in the morning.
Actually, I think I may have just used a normal Init when I first tried this and I don't think it triggered the test.
r=bholley - from comment 5

(In reply to Bobby Holley (:bholley) from comment #5)
> > +    JS::Rooted<JSObject*> baseGlobal(cx, mScriptGlobalObject->GetGlobalJSObject());
> 
> Again, I'd prefer CurrentGlobalOrNull in these situations, because I think
> it gives us more flexibility in tweaking the AutoJSAPI/nsIGlobalObject
> setup, and because GetGlobalJSObject() is technically fallible, whereas
> CurrentGlobalOrNull is not.

Done ... sorry, I wrote this patch before the comments on the previous one.
I'll try and remember this now.
 
> @@ +3704,5 @@
> > +    // AutoEntryScript. This is Gecko specific and not in any spec.
> > +    AutoEntryScript aes(mScriptGlobalObject);
> > +    JSContext* cx = aes.cx();
> > +    JS::Rooted<JSObject*> baseGlobal(cx, mScriptGlobalObject->GetGlobalJSObject());
> > +    NS_ENSURE_TRUE(baseGlobal, NS_ERROR_FAILURE);
> 
> And with that, we don't need this. AutoEntryScript currently just asserts
> that GetGlobalJSObject() doesn't return null.

Removed.
Attachment #8467114 - Attachment is obsolete: true
Attachment #8467826 - Flags: review+
(In reply to Bobby Holley (:bholley) from comment #12)
> Comment on attachment 8467139 [details] [diff] [review]
> Part 4: Replace nsCxPusher in nsDocShell::AddState v1
> 
> Review of attachment 8467139 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Er, I meant to rminus this. But after doing some reading, it looks like the
> structured clone algorithm _can_ in fact run script. I stand corrected.

After hunting again I found it mentioned in Step 8 - is this what you are referring to?
If it is, should the spec mention jumping to a script entry point here?

> This is really error prone though with the current InitWithJSVal API though,
> because it means that the caller has to be sure to use an AutoEntryScript.
> So I think we should change the InitWithJSVal API (which doesn't have a lot
> of consumers anyway) to stop taking a cx. If the jsval is a primitive, we
> just use an AutoJSAPI. If it's an object, we use an AutoEntryScript for that
> object's global.
> 
> Does that seem reasonable?

I think so, as the structured cloning enters the compartment of whatever it's cloning anyway.

Hopefully my implementation at least comes close to something reasonable. :)

I disappeared down a JS::Value rabbit hole for a while today, to try and understand some of what's going on here.
As we're no longer being passed a context here and the only compartment we're going to enter is the one for |aData|, I don't think the initial wrapping is needed any more.

For the primitive path, it doesn't look like the context is used unless it's a string and even then, I don't think it cares at all about the compartment (but I'm not sure).

I've run some tests locally and I've just kicked off another try push:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=06caff307e78
Attachment #8467139 - Attachment is obsolete: true
Attachment #8467852 - Flags: review?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #13)

> Hm - which tests are these? Are they related to error reporting, or is the
> goal just to keep the same behavior here?

The tests seems to be permission, not error reporting related and they pass with a normal Init (and therefore the SafeJSContext).

I did try this originally, don't know why I changed it to the legacy error reporting one.
Attachment #8467145 - Attachment is obsolete: true
Attachment #8467857 - Flags: review?(bobbyholley)
(In reply to Bobby Holley (:bholley) from comment #8)

> XBL installation runs at crazy times (like during layout), so I think we
> basically never want to run script there (and if we ever did in fact trigger
> a setter, I would much prefer us to throw than to run it). Can you switch to
> an AutoJSAPI and describe as much in the comment?

Still waiting on some Windows Try tests for this, which seem to have a massive queue at the moment. :(
(In reply to Bob Owen (:bobowen) from comment #18)
> After hunting again I found it mentioned in Step 8 - is this what you are
> referring to?

Yes - it invokes [[Get]]. Also, see https://mail.mozilla.org/pipermail/es-discuss/2011-July/015986.html .

> If it is, should the spec mention jumping to a script entry point here?

Almost certainly! Please file a bug on Hixie's tracker and CC me.

> I disappeared down a JS::Value rabbit hole for a while today, to try and
> understand some of what's going on here.
> As we're no longer being passed a context here and the only compartment
> we're going to enter is the one for |aData|, I don't think the initial
> wrapping is needed any more.

Right. We should just serialize directly in the compartment of the value.

> For the primitive path, it doesn't look like the context is used unless it's
> a string and even then, I don't think it cares at all about the compartment
> (but I'm not sure).

Yeah. AutoJSAPI should be fine there.
Comment on attachment 8467852 [details] [diff] [review]
Part 4: Replace nsCxPusher in nsDocShell::AddState v2

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

Awesome! Thanks for doing this.
Attachment #8467852 - Flags: review?(bobbyholley) → review+
Attachment #8467857 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #8)
> Comment on attachment 8467117 [details] [diff] [review]
> Part 2: Replace AutoCxPusher in nsXBLProtoImpl::InstallImplementation v1
> 
> Review of attachment 8467117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> XBL installation runs at crazy times (like during layout), so I think we
> basically never want to run script there (and if we ever did in fact trigger
> a setter, I would much prefer us to throw than to run it). Can you switch to
> an AutoJSAPI and describe as much in the comment?

I've changed this to an AutoJSAPI and added a comment as to why.

I've also changed to just checking to make sure we have an outer window, rather that leaving in the old checks for script global and script context.
I figured that this was probably why we were sometimes getting a null script context and this seems to fix the leak problems in crash and ref tests:
https://tbpl.mozilla.org/?tree=Try&rev=d0a711fde85e
Attachment #8467117 - Attachment is obsolete: true
Attachment #8468516 - Flags: review?(bobbyholley)
Attachment #8468516 - Flags: review?(bobbyholley) → review+
Try push without the latest change to Part 2:
https://tbpl.mozilla.org/?tree=Try&rev=06caff307e78

Smaller try push with the final change to Part 2 (including the Windows crash and ref tests that had failures with some previous versions):
https://tbpl.mozilla.org/?tree=Try&rev=d0a711fde85e

Please land in part number order, thanks.
Keywords: checkin-needed
(In reply to Bobby Holley (:bholley) from comment #21)
> (In reply to Bob Owen (:bobowen) from comment #18)

> > If it is, should the spec mention jumping to a script entry point here?
> 
> Almost certainly! Please file a bug on Hixie's tracker and CC me.

I went to do this, but I couldn't work out how to word it.
The steps at [1] don't seem to lend themselves to this situation.
Would we be saying something like ... 
jump to a code entry point with script set to the script that created input?

or maybe

prepare to run a callback with the script settings object associated with input?

Thanks for the reviews, by the way.

[1] http://www.whatwg.org/specs/web-apps/current-work/#jump-to-a-code-entry-point
Flags: needinfo?(bobbyholley)
(In reply to Bob Owen (:bobowen) from comment #25)
> (In reply to Bobby Holley (:bholley) from comment #21)
> > (In reply to Bob Owen (:bobowen) from comment #18)
> 
> > > If it is, should the spec mention jumping to a script entry point here?
> > 
> > Almost certainly! Please file a bug on Hixie's tracker and CC me.
> 
> I went to do this, but I couldn't work out how to word it.
> The steps at [1] don't seem to lend themselves to this situation.
> Would we be saying something like ... 
> jump to a code entry point with script set to the script that created input?
> 
> or maybe
> 
> prepare to run a callback with the script settings object associated with
> input?

This one, or something like it.

I guess the spec is currently well-defined, assuming that there are no places that we can trigger a structured clone when we're not already running script. I still think we should fix it though, because it makes implementations much simpler (since we can push the entry point when we need it, and not have to worry about inheriting it from some far-away place).

Anyway, let's just get a spec bug on file and talk about it there.
Flags: needinfo?(bobbyholley)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: