Boris Zbarsky [:bzbarsky] Reporter
	Description • 12 years ago

See bug 840201

	Florian Bender
	Comment 1 • 12 years ago

I don't know if this is relevant, but I want to point out that the Mixed Active Content Blocker feature (bug 815321) may disable execution of scripts for a given global at some point (AFAIU).

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 2 • 12 years ago

As long as it does it "globally" for a given global, it's not a problem.

	Florian Bender
	Comment 3 • 12 years ago

You should check that with the responsible person. AFAIK, JS execution permission can be revoked at any time during or after page load (e. g. a mixed XHR), and also just for specific scripts and e. g. iFrames (not the document itself). But I don't know how the internals work here, I just wanted to point that out.

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

I'm working on this.

	Bobby Holley (:bholley) Assignee
	Comment 5 • 11 years ago

CCing Giorgio, who's going to want to keep a close eye on this bug and make sure it doesn't break NoScript.

	Bobby Holley (:bholley) Assignee
	Comment 6 • 11 years ago

So, I've been looking at the current pref-based mechanism for blocking script for a given domain, and it's pretty clunky. It requires setting:

capability.policy.somepolicyname.sites = /* some list of sites */
and then setting
capability.polcy.somepolicyname.javascript.enabled = false

If we want to do this as a dynamic lookup during document creation, it seems like we'd want something leaner. What sort of API would it make sense to provide consumers like NoScript? I think it'd be nice to just provide access to an in-memory hashtable (keyed off principals) and leave persistent storage up to the embedder. Thoughts?

	Giorgio Maone [:ma1]
	Comment 7 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #6)
> So, I've been looking at the current pref-based mechanism for blocking
> script for a given domain, and it's pretty clunky. It requires setting:
> 
> capability.policy.somepolicyname.sites = /* some list of sites */
> and then setting
> capability.polcy.somepolicyname.javascript.enabled = false

Yes, I don't know why the storage was the preference system (likely because it was the simplest available at the time CAPS was implemented), but the extra clunkiess is due also to the (excessive?) flexibility of this mechanism, which allowed limiting JavaScript access with property-level granularity (e.g. you could veto window.open on some domains).

> If we want to do this as a dynamic lookup during document creation, it seems
> like we'd want something leaner.

At this moment the preferences are translated by the ScriptSecurityManager component into hashtables (keyed by policies and principals) every time a change is observed.
This seems fairly efficient, but there's no API at all exposed to add-ons like NoScript or the XUL front-end to access these data structure. Therefore NoScript is forced to replicate the hashtable in JavaScript land in order to access permission information quicly when needed.
 
> What sort of API would it make sense to
> provide consumers like NoScript? I think it'd be nice to just provide access
> to an in-memory hashtable (keyed off principals) and leave persistent
> storage up to the embedder. Thoughts?

It would be great for the reasons told above, if not else to remove the data structures redundancy and its memory waste.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 8 • 11 years ago

I'm fine with better API here... just worried about breaking existing extensions.  :(

	Giorgio Maone [:ma1]
	Comment 9 • 11 years ago

Side note: if a new declarative API for script permissions gets implemented (notice that we can use to some extent nsIDocShell.allowJavascript to do it programmatically), a way to handle per-private-window permissions sets might be desirable, as requested in http://forums.informaction.com/viewtopic.php?f=10&t=17144 -- even though not necessarily for the same reasons.

	Bobby Holley (:bholley) Assignee
	Comment 10 • 11 years ago

(In reply to Giorgio Maone from comment #9)
> Side note: if a new declarative API for script permissions gets implemented

I wasn't imagining that it would be declarative. Can you describe NoScript's requirements? In particular, I'm wondering whether:
* We need the ability to block arbitrary subdomains.
* We need the ability to implement both a whitelist and a blacklist.

> (notice that we can use to some extent nsIDocShell.allowJavascript to do it
> programmatically)

That doesn't work for blocking script from certain origins, does it? Or is the idea that you'd disable script globally, listen for new docshells, and allow it on a case-by-case basis?

, a way to handle per-private-window permissions sets might
> be desirable, as requested in
> http://forums.informaction.com/viewtopic.php?f=10&t=17144 -- even though not
> necessarily for the same reasons.

That sounds like followup work that I'm unlikely to get to myself, but would happily mentor.

	Giorgio Maone [:ma1]
	Comment 11 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #10)
> (In reply to Giorgio Maone from comment #9)
> > Side note: if a new declarative API for script permissions gets implemented
> 
> I wasn't imagining that it would be declarative. Can you describe NoScript's
> requirements? In particular, I'm wondering whether:
> * We need the ability to block arbitrary subdomains.

To block or to allow, yes.

> * We need the ability to implement both a whitelist and a blacklist.

Yes.

> > (notice that we can use to some extent nsIDocShell.allowJavascript to do it
> > programmatically)
> 
> That doesn't work for blocking script from certain origins, does it? Or is
> the idea that you'd disable script globally, listen for new docshells, and
> allow it on a case-by-case basis?

Yes that's the idea, even though IIRC the most obvious topic to be observed ("content-document-global-created") triggers notifications at a time when the HTML parser is already set up to ignore or honor <NOSCRIPT> elements, depending on the previous Javascript enablement state. Not sure whether this has changed with the HTML5 parser, will check as soon as I'm back from the Summit + OWASP Belgium Chapter, by the end of the week.

	Bobby Holley (:bholley) Assignee
	Comment 12 • 11 years ago

At long last:

https://tbpl.mozilla.org/?tree=Try&rev=3f1b2c5ac55d

	Bobby Holley (:bholley) Assignee
	Comment 13 • 11 years ago

s/nsTHashTable.h/nsTHashtable.h/ :-(

https://tbpl.mozilla.org/?tree=Try&rev=68f670dfa95a

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

https://tbpl.mozilla.org/?tree=Try&rev=c0a3fdd35fb9

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

https://tbpl.mozilla.org/?tree=Try&rev=35a5b80e72d5

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

green.

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

The old code seemed to feel that the lack of a script context was some sort of
showdown that required either unconditional allow or deny. Instead, let's just
make the scriptcontext-relevant checks conditional on there being a script
context, which lets us switch CheckFunctionAccess over to ScriptAllowed.

	Bobby Holley (:bholley) Assignee
	Comment 19 • 11 years ago

\o/

Aside from checking whether script is enabled, all the other work this function
does is silly.

	Bobby Holley (:bholley) Assignee
	Comment 20 • 11 years ago

The current tests never actually check that script is disabled for unsafe JAR
channels, but only that they're unable to initiate loads. This fixes that.

This blob runs dos2unix on the files inside the zip, and applies the following patch to iframes.html:

 <html><head>
   <title>iframes</title>
+  <script>parent.poke('regular script');</script>
 </head>
-<body>
+<body onload="parent.poke('onload-handler')">
  subshells
  <p>
  <iframe id="data-iframe" src="data:text/html,dataurl iframe<script>window.parent.parent.poke('data-iframe')</script>">ifr</iframe>
  <p>
  <iframe id="js-iframe" src="javascript:window.parent.parent.poke('js-iframe')">jsifr</iframe>
 </body>
-</html>
\ No newline at end of file
+</html>

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

This setup is kind of hacky. JSD exposed a per-context script toggle, which,
while nonsensical, exhibits visibly different semantics than a per-global
toggle (since the former follows the WindowProxy across navigations, whereas
the latter does not). Honza says that Firebug ensures that any instances of
|jsdcx.scriptsEnabled = false| are guaranteed to be followed by
|jsdcx.scriptsEnabled = true| before a navigation occurs, so this should be
good enough.

	Bobby Holley (:bholley) Assignee
	Comment 26 • 11 years ago

There are now no longer any consumers that set this to false.

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

Note that the checks in nsJSEnvironment::EvaluateString and EvalInWindow
can safely go away, because we call ssm->ScriptAllowed() in
nsJSUtils::EvaluateString.

	Bobby Holley (:bholley) Assignee
	Comment 29 • 11 years ago

Note that this patch changes the semantics of javascript.enabled so that changes
to the pref do not apply to compartments that have already been created. This is
a significant change, but is necessary to support the new domain policy API.
After one cycle or so, we'll rip out the old API.

	Bobby Holley (:bholley) Assignee
	Comment 31 • 11 years ago

Comment on attachment 828472 [details] [diff] [review]
Part 13 - New domain policy API. v1

Giorgio - this is the new API for NoScript (see also the tests in part 14). Does it meet your needs?

	Giorgio Maone [:ma1]
	Comment 32 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #31)
> Comment on attachment 828472 [details] [diff] [review]
> Part 13 - New domain policy API. v1
> 
> Giorgio - this is the new API for NoScript (see also the tests in part 14).
> Does it meet your needs?

If I understand it right, NoScript should grab the ScriptSecurityManager to initialize its new nsIDomainSet properties every time the browser is started, and manage the data persistence across session on its own, correct?

Anyway, looks good.

	Bobby Holley (:bholley) Assignee
	Comment 33 • 11 years ago

(In reply to Giorgio Maone from comment #32)
> If I understand it right, NoScript should grab the ScriptSecurityManager to
> initialize its new nsIDomainSet properties every time the browser is
> started, and manage the data persistence across session on its own, correct?

That is correct. Note that you'll also have to flip on domainPolicyEnabled to trigger the creation of the hashtables.

> Anyway, looks good.

Great! How much overlap / lead time do you need? I'd originally planned one cycle of overlap between the old and the new API, though I'm now wondering whether we should do it all in one cycle, since these patches already alter the semantics of the old API.

	Giorgio Maone [:ma1]
	Comment 34 • 11 years ago

One overlap cycle at least would be highly desirable: NoScript users take script blocking reliability for granted and they couldn't tolerate surprises.

Also, on a second look, I'll likely need:

1) A mean to enumerate nsIDomainSet existing items
2) A mean to be notified of nsIDomainSet changes (as an observer?)

Both features are useful to ensure consistency between NoScript's persistence / UI layers and the actual permissions, and to become aware as soon as possible of conflicts with other potential clients of this API, in order to notify users that their current setup is unreliable.

	Bobby Holley (:bholley) Assignee
	Comment 35 • 11 years ago

Hm, maybe it would be better to explicitly require that this policy is only used by one consumer? It seems like multiple addons trying to mess with the domain policy is just asking for trouble.

How about we do this:

(1) Add a new interface, nsIDomainPolicy.
(2) Move the 4 nsIDomainSets from nsIScriptSecurityManager to nsIDomainPolicy.
(3) Create a method on nsIScriptSecurityManager called activateDomainPolicy(). This returns an nsIDomainPolicy to the caller. The nsIDomainPolicy is not accessible by any other mechanism from script, so it's private to the caller.
(4) Make domainPolicyActive [readonly] on nsIScriptSecurityManager. When it is true, subsequent calls to activateDomainPolicy() will fail. When a caller invokes deactivate() on its nsIDomainPolicy, domainPolicyActive becomes false, and someone new may acquire ownership of the domain policy.

How does this sound Giorgio?

	Bobby Holley (:bholley) Assignee
	Comment 36 • 11 years ago

Comment on attachment 828472 [details] [diff] [review]
Part 13 - New domain policy API. v1

Canceling review on the domain policy stuff, since it sounds like we'll need to modify it a bit based on Giorgio's feedback. All the other patches are still reviewable.

	Giorgio Maone [:ma1]
	Comment 37 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #35)
> Hm, maybe it would be better to explicitly require that this policy is only
> used by one consumer? It seems like multiple addons trying to mess with the
> domain policy is just asking for trouble.

In facts, my first instinct was asking for some "exclusive access" mechanism like the one you described, but it seemed asking too much :)

> How about we do this:
> 
> (1) Add a new interface, nsIDomainPolicy.
> (2) Move the 4 nsIDomainSets from nsIScriptSecurityManager to
> nsIDomainPolicy.
> (3) Create a method on nsIScriptSecurityManager called
> activateDomainPolicy(). This returns an nsIDomainPolicy to the caller. The
> nsIDomainPolicy is not accessible by any other mechanism from script, so
> it's private to the caller.
> (4) Make domainPolicyActive [readonly] on nsIScriptSecurityManager. When it
> is true, subsequent calls to activateDomainPolicy() will fail. When a caller
> invokes deactivate() on its nsIDomainPolicy, domainPolicyActive becomes
> false, and someone new may acquire ownership of the domain policy.
> 
> How does this sound Giorgio?

Sounds excellent (we can fail at init time on conflict).
But please add also items enumeration and change notifications, if possible.
Thank you.

	Bobby Holley (:bholley) Assignee
	Comment 38 • 11 years ago

(In reply to Giorgio Maone from comment #37)
> But please add also items enumeration and change notifications, if possible.

Well, both of those add extra goop to this code. Why do you need those if you have exclusive access to the system?

	Giorgio Maone [:ma1]
	Comment 39 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #38)
> (In reply to Giorgio Maone from comment #37)
> > But please add also items enumeration and change notifications, if possible.
> 
> Well, both of those add extra goop to this code. Why do you need those if
> you have exclusive access to the system?

In order to avoid duplicating volatile data structures, rather than just taking on persistence responsibility.
Is this a real bloat issue, or am I just worrying for nothing (e.g. are nsIURI representations canonicalized some way)?
BTW, in face of whitelists with thousands of entries (not uncommon among NoScript users), can we figure how startup overhead from the repeated add() JavaScript->XPCOM calls would look like?

	Bobby Holley (:bholley) Assignee
	Comment 40 • 11 years ago

(In reply to Giorgio Maone from comment #39)
> In order to avoid duplicating volatile data structures, rather than just
> taking on persistence responsibility.

I don't see how observers would be helpful here.

> Is this a real bloat issue, or am I just worrying for nothing (e.g. are
> nsIURI representations canonicalized some way)?
> BTW, in face of whitelists with thousands of entries (not uncommon among
> NoScript users), can we figure how startup overhead from the repeated add()
> JavaScript->XPCOM calls would look like?

nsIURIs are cloned when passed to and from the domain sets, to avoid the issue where someone might mutate an nsIURI and alter the script policy without realizing it.

Providing access to the URIs in the DomainSet is going to be a pain - we'd probably have to make copies of various nsIURIs, and return a giant array or something. nsIURIs are much more heavyweight than strings, because of the overhead of having an underlying XPCOM object. I think NoScript should maintain an in-memory representation in string form, and generate the relevant nsIURIs on demand to manipulate the script policy.

	Bobby Holley (:bholley) Assignee
	Comment 41 • 11 years ago

Updated to the new design for the domain policy.

	Giorgio Maone [:ma1]
	Comment 43 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #40)
> (In reply to Giorgio Maone from comment #39)
> > In order to avoid duplicating volatile data structures, rather than just
> > taking on persistence responsibility.
> 
> I don't see how observers would be helpful here.

Fair. If the client is unique and nobody else can touch the DomainSets, observers are unneeded.

> Providing access to the URIs in the DomainSet is going to be a pain - we'd
> probably have to make copies of various nsIURIs, and return a giant array or
> something. nsIURIs are much more heavyweight than strings, because of the
> overhead of having an underlying XPCOM object. 

... or your new API could itself trade in strings, rather than URIs, and avoid cloning at all.

especially in the super(Whitelist|Blacklist) DomainSets, handling URIs when we're actually interested in domains seems a bit weird. 

> I think NoScript should
> maintain an in-memory representation in string form, and generate the
> relevant nsIURIs on demand to manipulate the script policy.

OK, let's start trying this way and see whether it causes any major performance issue, especially on startup. If it does, in future we might want to add a bulk add() method taking a giant string to be split or an array of strings and doing the heavy lift in C++, maybe.

	Bobby Holley (:bholley) Assignee
	Comment 44 • 11 years ago

(In reply to Giorgio Maone from comment #43)
> ... or your new API could itself trade in strings, rather than URIs, and
> avoid cloning at all.

The strings still need to be canonicalized, which would generally require cloning anyway, and be much more likely to have subtle bugs.

Moreover, using an nsIURI in C++ is much less expensive (relatively) than using it from JS.

> especially in the super(Whitelist|Blacklist) DomainSets, handling URIs when
> we're actually interested in domains seems a bit weird. 

Note that for both super and non-super variants, the only fields of interest are protocol, host, and port. All the other fields are cleared during the clone.

> OK, let's start trying this way and see whether it causes any major
> performance issue, especially on startup. If it does, in future we might
> want to add a bulk add() method taking a giant string to be split or an
> array of strings and doing the heavy lift in C++, maybe.

String processing is generally faster in JS than in C++, and much less likely to introduce security vulnerabilities.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 45 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #29)
> Note that this patch changes the semantics of javascript.enabled so that
> changes
> to the pref do not apply to compartments that have already been created.
> This is
> a significant change, but is necessary to support the new domain policy API.
> After one cycle or so, we'll rip out the old API.

Not sure what kind of feedback I should give.

- Does the change also require code changes on Firebug side?
- Or should we just test Firebug with the patch (if yes, can we get a try build?)

Honza

	Bobby Holley (:bholley) Assignee
	Comment 46 • 11 years ago

(In reply to Jan Honza Odvarko from comment #45)
> Not sure what kind of feedback I should give.
> 
> - Does the change also require code changes on Firebug side?
> - Or should we just test Firebug with the patch (if yes, can we get a try
> build?)

I think it should be fine, but I just wanted you to be aware of the altered semantics here. You can use the try build in comment 15 if you like, or you can just wait until it lands.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 47 • 11 years ago

(In reply to Bobby Holley (:bholley) from comment #46)
> I think it should be fine, but I just wanted you to be aware of the altered
> semantics here. You can use the try build in comment 15 if you like, or you
> can just wait until it lands.
OK, I'll be testing with the Nightly build as soon as it's available,
thanks for the heads up!

Honza

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 48 • 11 years ago

Comment on attachment 828459 [details] [diff] [review]
Part 1 - Refactor Gecko to provide a more direct API to ask whether script is allowed for a given global. v1

nostdcall too for the IDL, right?  Or NS_IMETHODIMP_(bool) in the .cpp.

>+    AutoPushJSContext cx(scx ? scx->GetNativeContext() : GetSafeJSContext());
>+    JS::RootedObject global(cx, aGlobal);

I believe that's a rooting hazard, because the autopush stuff can gc.  :(  Please fix that.

>+++ b/content/base/src/nsDocument.cpp
>@@ -7202,28 +7202,19 @@ nsDocument::IsScriptEnabled()

Don't we want to use the inner window here, not the outer (which might not even match our principals, if we're not the current doc)?

>+++ b/content/html/document/src/nsHTMLContentSink.cpp

Similar here.

>+++ b/content/xbl/src/nsXBLBinding.cpp

And here.

>+++ b/parser/html/nsHtml5TreeOpExecutor.cpp

And here.

r=me with those issues fixed.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 49 • 11 years ago

Comment on attachment 828460 [details] [diff] [review]
Part 2 - Get rid of aAllowIfNoScriptContext. v1

r=me, I think.  Let's hope no one was depending on script being disallowed where we'll now allow it...

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 50 • 11 years ago

Comment on attachment 828461 [details] [diff] [review]
Part 3 - Remove CheckFunctionAccess. v1

r=hurray!

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 51 • 11 years ago

Comment on attachment 828462 [details] [diff] [review]
Part 4 - Update unsafe-JAR channel tests so that they actually fail if we allow script to run. v1

I'm not sure I follow.  Why was the old test not testing whether the <script> runs?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 52 • 11 years ago

Comment on attachment 828463 [details] [diff] [review]
Part 5 - Introduce a mechanism to temporarily or permanently block script for a given scope, and use it for unsafe channels. v1

> Note that we assume that |cx|
>+    // is in the relevant compartment here.

I don't follow.  There is no cx involved in that check.

>+++ b/dom/base/nsGlobalWindow.cpp

Why don't we need to do this in the reuse-inner-window case?  Seems to me like we should, in fact!

>+++ b/js/xpconnect/idl/xpccomponents.idl

More documentation here, please.  The API does not make it obvious that this is a counter.

r=me with the above fixed

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 53 • 11 years ago

Comment on attachment 828464 [details] [diff] [review]
Part 6 - Directly mark compartments whose docshells disable script execution. v1

Hmm.  The new check for mTreeOwner wasn't done at all before, right?  I think it's ok, but why did you need to add that?

>+nsDocShell::RecomputeScriptability()

Maybe RecomputeCanExecuteScripts?  I don't feel super-strongly about it, though.

>+            nsCOMPtr<nsIDocShell> child = do_QueryObject(iter.GetNext());
>+            static_cast<nsDocShell*>(child.get())->RecomputeScriptability();

Just static_cast<nsDocShell*>(iter.GetNext())->RecomputeScriptability() seems equivalent.

>@@ -2421,16 +2424,20 @@ nsGlobalWindow::SetNewDocument(nsIDocument* aDocument,

I assume this part also handles things coming out of bfcache?

>+++ b/js/xpconnect/src/XPCJSRuntime.cpp
>+    return mDocShellAllowsScript && (mScriptBlocks == 0);

Why the extra parens?

r=me

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 54 • 11 years ago

Comment on attachment 828465 [details] [diff] [review]
Part 7 - Move print system script disabling to the new API. v1

Again, this should be working with the document's inner window, not the outer.

r=me with that

	Bobby Holley (:bholley) Assignee
	Comment 55 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #51)
> I'm not sure I follow.  Why was the old test not testing whether the
> <script> runs?

I don't know, but when I commented out the UnsafeChannel stuff in nsDocShell.cpp, the test still passed. With this patch, the tests fail when I do that.

	Bobby Holley (:bholley) Assignee
	Comment 56 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #52)
> Why don't we need to do this in the reuse-inner-window case?  Seems to me
> like we should, in fact!

I don't follow - AFAICT this will be called in both cases (though only if !aState).

	Bobby Holley (:bholley) Assignee
	Comment 57 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #53)
> Hmm.  The new check for mTreeOwner wasn't done at all before, right?  I
> think it's ok, but why did you need to add that?

In order to disambiguate the cases of being the root docshell and being a detached docshell, right?

> Maybe RecomputeCanExecuteScripts?  I don't feel super-strongly about it,
> though.

ok.

> >@@ -2421,16 +2424,20 @@ nsGlobalWindow::SetNewDocument(nsIDocument* aDocument,
> 
> I assume this part also handles things coming out of bfcache?

Yes. It doesn't run for the inner window reuse case, but I think that's ok.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 58 • 11 years ago

> when I commented out the UnsafeChannel stuff in nsDocShell.cpp, the test still passed.

Would you mind checking why?  That part doesn't make sense to me, and that worries me.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 59 • 11 years ago

> I don't follow - AFAICT this will be called in both cases (though only if !aState).

You're right; I misread the diff, sorry.  :(

> In order to disambiguate the cases of being the root docshell and being a detached
> docshell, right?

Oh, to avoid falling all the way to the trailing else.  That's worth documenting explicitly, I think.

Still, that's a behavior change in that we used to allow script in detached docshells, in theory (except they never had a script global, I guess) right?

> Yes. It doesn't run for the inner window reuse case, but I think that's ok.

In the inner window reuse case we keep the same docshell and the same compartment, so yeah, this should be fine.

	Bobby Holley (:bholley) Assignee
	Comment 60 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #58)
> > when I commented out the UnsafeChannel stuff in nsDocShell.cpp, the test still passed.
> 
> Would you mind checking why?  That part doesn't make sense to me, and that
> worries me.

What do you mean "checking why"? As noted in comment 20, the tests were written to check for initiated loads (via data and javascript urls that poke the parent), but it never actually checks that the frame itself can't run script.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 61 • 11 years ago

Comment on attachment 828462 [details] [diff] [review]
Part 4 - Update unsafe-JAR channel tests so that they actually fail if we allow script to run. v1

Oh, I'm just on crack again.  I thought you were _removing_ the inline script.  This totally makes sense.

	Bobby Holley (:bholley) Assignee
	Comment 62 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #59)
> Oh, to avoid falling all the way to the trailing else.  That's worth
> documenting explicitly, I think.

Something other than the existing comment above it? Currently it reads:

// If we have no tree owner, that means that we've been detached from the
// docshell tree. In that case, don't allow script.
 
> Still, that's a behavior change in that we used to allow script in detached
> docshells, in theory (except they never had a script global, I guess) right?

Well, they have have had script globals and been detached due to a navigation in a parent dochshell. But we were already freezing them in that case, and preventing event handlers and timeouts and whatnot. This is just making it a bit more consistent.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 63 • 11 years ago

> Something other than the existing comment above it?

Yes.  I think we should explicitly say that we're checking this so we won't think this is a toplevel docshell for which script should be enabled case.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 64 • 11 years ago

Comment on attachment 828466 [details] [diff] [review]
Part 8 - Move jsd over to the new API. v1

r=me.  The timeout handling is subtly different, but I think it should be close enough for Firebug's purposes.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 65 • 11 years ago

Comment on attachment 828467 [details] [diff] [review]
Part 9 - Move nsXULDocument to new-style scripts checks. v1

r=me

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 66 • 11 years ago

Comment on attachment 828468 [details] [diff] [review]
Part 10 - Remove nsIScriptContext script check in nsScriptLoader. v1

r=me

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 67 • 11 years ago

Comment on attachment 828470 [details] [diff] [review]
Part 11 - Remove per-JSContext script toggling. v1

r=me, and good riddance.  ;)

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 68 • 11 years ago

Comment on attachment 828471 [details] [diff] [review]
Part 12 - Compute immunity from caps checks exactly once, and kill nsScriptSecurityManager::CanExecuteScripts. v1

r=me

This patch makes it obvious that part 5 changed things so that system and expanded principals are not opted out of explicitly-disabled-on-the-compartment script anymore.  Per irc discussion, let's try that and see what happens.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 70 • 11 years ago

Comment on attachment 829043 [details] [diff] [review]
Part 13 - New domain policy API. v2

Need to rev the security manager IID.

This diff is missing DomainPolicy.h

You should probably document for all the methods taking URIs that they will only use the scheme+host+port of the URI and further that the domain policy APIs will make copies of the passed-in URIs.

>+namespace hotness {

Cute, but let's not?  I think just mozilla::DomainPolicy is fine.

>+    rv = clone->SetUserPass(NS_LITERAL_CSTRING(""));

EmptyCString().  Same for SetPath.

Looks fine otherwise, but I'd like to see the header before I mark r+.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 71 • 11 years ago

Comment on attachment 829044 [details] [diff] [review]
Part 14 - Tests. v2

I mostly skimmed here.  Let me know if you want me to read more carefully.

	Bobby Holley (:bholley) Assignee
	Comment 72 • 11 years ago

Now with DomainPolicy.h.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 73 • 11 years ago

So without the patches here, on http://mozilla.pettay.fi/moztests/events/event_speed_2.1.html with depth set to 1 I get:

avg of 30 results (total tests 50) : 57.13333333333333ms

and on http://mozilla.pettay.fi/moztests/events/event_speed_3.html (depth 1) I get:

avg of 30 results (total tests 50) : 136.56666666666666ms

With the patch I get:

avg of 30 results (total tests 50) : 45.56666666666667ms

and

avg of 30 results (total tests 50) : 69.7ms

respectively.

For the "3" testcase, the time spent under the CallSetup constructor went from 61% CheckFunctionAccess to 29% ScriptAllowed.

The time under ScriptAllowed is 56% LookupPolicy 15% AutoJSContext, and the rest is long-tailish and self time.  So hopefully this will still get a bit faster.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 74 • 11 years ago

Comment on attachment 829658 [details] [diff] [review]
Part 13 - New domain policy API. v3

r=me

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 75 • 11 years ago

Comment on attachment 828464 [details] [diff] [review]
Part 6 - Directly mark compartments whose docshells disable script execution. v1

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +435,5 @@
>  
> +void
> +Scriptability::SetDocShellAllowsScript(bool aAllowed)
> +{
> +    mDocShellAllowsScript = aAllowed;;

;;

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 76 • 11 years ago

Comment on attachment 828471 [details] [diff] [review]
Part 12 - Compute immunity from caps checks exactly once, and kill nsScriptSecurityManager::CanExecuteScripts. v1

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

::: caps/idl/nsIScriptSecurityManager.idl
@@ +203,5 @@
>      boolean isSystemPrincipal(in nsIPrincipal aPrincipal);
> +%{C++
> +    bool IsSystemPrincipal(nsIPrincipal* aPrincipal) {
> +      bool isSystem = false;
> +      IsSystemPrincipal(aPrincipal, &isSystem);

assert that it didn't fail, please

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 77 • 11 years ago

Isn't nsIDomainPolicy rather vague name. If I just read that name, or read the interface (without comments), it doesn't hint in anyway that it is about scripting.
nsIDomainScriptingPolicy perhaps?

	Bobby Holley (:bholley) Assignee
	Comment 78 • 11 years ago

(In reply to :Ms2ger from comment #76)
> assert that it didn't fail, please

I don't want to much around with DebugOnly in an idl file.

	Bobby Holley (:bholley) Assignee
	Comment 79 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=c99c82511097

	Bobby Holley (:bholley) Assignee
	Comment 80 • 11 years ago

Green.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=cd0e2e0ef136

	Bobby Holley (:bholley) Assignee
	Comment 81 • 11 years ago

Giorgio, when this hits nightly, please give it a try. If I don't hear any issues, I'll proceed with removing the old mechanism in mozilla 29.

	Ryan VanderMeulen [:RyanVM]
	Comment 82 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/9bff006f8fda
https://hg.mozilla.org/mozilla-central/rev/4f3f338af4c7
https://hg.mozilla.org/mozilla-central/rev/3c6edeaccb47
https://hg.mozilla.org/mozilla-central/rev/9f0e23094b73
https://hg.mozilla.org/mozilla-central/rev/52a8082a281f
https://hg.mozilla.org/mozilla-central/rev/a0a49b75b8aa
https://hg.mozilla.org/mozilla-central/rev/92ab5fa19429
https://hg.mozilla.org/mozilla-central/rev/f059a2f80c22
https://hg.mozilla.org/mozilla-central/rev/d3d9d89c80ef
https://hg.mozilla.org/mozilla-central/rev/2ed06289d216
https://hg.mozilla.org/mozilla-central/rev/73fd5de6e820
https://hg.mozilla.org/mozilla-central/rev/4beeac0fbc68
https://hg.mozilla.org/mozilla-central/rev/9f9022aabfe9
https://hg.mozilla.org/mozilla-central/rev/cd0e2e0ef136

	Mark Straver
	Comment 84 • 11 years ago

Is there going to be documentation available for the new API before it hits the release channel? If so, where?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 85 • 11 years ago

Mark, http://hg.mozilla.org/mozilla-central/file/29882306e8ca/caps/idl/nsIScriptSecurityManager.idl#l234 and http://hg.mozilla.org/mozilla-central/file/29882306e8ca/caps/idl/nsIDomainPolicy.idl#l12 pretty much document the entirety of the API, but you're right that devmo documentation might be nice.

	Bobby Holley (:bholley) Assignee
	Comment 86 • 11 years ago

I'm unclear on what info is required of me here. The stuff is well-documented in the idl (see comment 85). Anything else I'll leave up to the docs team.