Add a much faster way to do the "is script enabled for this global?" check

RESOLVED FIXED in mozilla28

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bz, Assigned: bholley)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla28
x86
Mac OS X
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(15 attachments, 3 obsolete attachments)

13.01 KB, patch
Details | Diff | Splinter Review
7.02 KB, patch
Details | Diff | Splinter Review
7.08 KB, patch
Details | Diff | Splinter Review
2.77 KB, patch
Details | Diff | Splinter Review
11.50 KB, patch
Details | Diff | Splinter Review
13.57 KB, patch
Details | Diff | Splinter Review
2.88 KB, patch
Details | Diff | Splinter Review
5.90 KB, patch
Details | Diff | Splinter Review
1.56 KB, patch
Details | Diff | Splinter Review
1.82 KB, patch
Details | Diff | Splinter Review
12.96 KB, patch
Details | Diff | Splinter Review
12.90 KB, patch
Details | Diff | Splinter Review
15.91 KB, patch
Details | Diff | Splinter Review
101.48 KB, patch
Details | Diff | Splinter Review
20.67 KB, patch
Details | Diff | Splinter Review
See bug 840201
Blocks: 840201

Comment 1

4 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).
As long as it does it "globally" for a given global, it's not a problem.

Comment 3

4 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.
Blocks: 913734
I'm working on this.
Assignee: nobody → bobbyholley+bmo
CCing Giorgio, who's going to want to keep a close eye on this bug and make sure it doesn't break NoScript.
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?
Flags: needinfo?(g.maone)
Flags: needinfo?(bzbarsky)

Comment 7

4 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.
Flags: needinfo?(g.maone)
Blocks: 807371
I'm fine with better API here... just worried about breaking existing extensions.  :(
Flags: needinfo?(bzbarsky)

Comment 9

4 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.
(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.
Flags: needinfo?(g.maone)
(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.
Flags: needinfo?(g.maone)
At long last:

https://tbpl.mozilla.org/?tree=Try&rev=3f1b2c5ac55d
s/nsTHashTable.h/nsTHashtable.h/ :-(

https://tbpl.mozilla.org/?tree=Try&rev=68f670dfa95a
https://tbpl.mozilla.org/?tree=Try&rev=c0a3fdd35fb9
https://tbpl.mozilla.org/?tree=Try&rev=35a5b80e72d5
green.
Created 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
Attachment #828459 - Flags: review?(bzbarsky)
Created attachment 828460 [details] [diff] [review]
Part 2 - Get rid of aAllowIfNoScriptContext. v1

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.
Attachment #828460 - Flags: review?(bzbarsky)
Created attachment 828461 [details] [diff] [review]
Part 3 - Remove CheckFunctionAccess. v1

\o/

Aside from checking whether script is enabled, all the other work this function
does is silly.
Attachment #828461 - Flags: review?(bzbarsky)
Created attachment 828462 [details] [diff] [review]
Part 4 - Update unsafe-JAR channel tests so that they actually fail if we allow script to run. v1

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>
Attachment #828462 - Flags: review?(bzbarsky)
Created 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
Attachment #828463 - Flags: review?(bzbarsky)
Created attachment 828464 [details] [diff] [review]
Part 6 - Directly mark compartments whose docshells disable script execution. v1
Attachment #828464 - Flags: review?(bzbarsky)
Created attachment 828465 [details] [diff] [review]
Part 7 - Move print system script disabling to the new API. v1
Attachment #828465 - Flags: review?(bzbarsky)
Created attachment 828466 [details] [diff] [review]
Part 8 - Move jsd over to the new API. v1

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.
Attachment #828466 - Flags: review?(bzbarsky)
Created attachment 828467 [details] [diff] [review]
Part 9 - Move nsXULDocument to new-style scripts checks. v1
Attachment #828467 - Flags: review?(bzbarsky)
Created attachment 828468 [details] [diff] [review]
Part 10 - Remove nsIScriptContext script check in nsScriptLoader. v1

There are now no longer any consumers that set this to false.
Attachment #828468 - Flags: review?(bzbarsky)
Created attachment 828470 [details] [diff] [review]
Part 11 - Remove per-JSContext script toggling. v1

Note that the checks in nsJSEnvironment::EvaluateString and EvalInWindow
can safely go away, because we call ssm->ScriptAllowed() in
nsJSUtils::EvaluateString.
Attachment #828470 - Flags: review?(bzbarsky)
Created attachment 828471 [details] [diff] [review]
Part 12 - Compute immunity from caps checks exactly once, and kill nsScriptSecurityManager::CanExecuteScripts. v1
Attachment #828471 - Flags: review?(bzbarsky)
Created attachment 828472 [details] [diff] [review]
Part 13 - New domain policy API. v1

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.
Attachment #828472 - Flags: review?(bzbarsky)
Created attachment 828473 [details] [diff] [review]
Part 14 - Tests. v1
Attachment #828473 - Flags: review?(bzbarsky)
Attachment #828466 - Flags: feedback?(odvarko)
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?
Attachment #828472 - Flags: feedback?(g.maone)
(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.
(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.
Flags: needinfo?(g.maone)
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.
Flags: needinfo?(g.maone)
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?
Flags: needinfo?(g.maone)
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.
Attachment #828472 - Flags: review?(bzbarsky)
Attachment #828472 - Flags: feedback?(g.maone)
Attachment #828473 - Flags: review?(bzbarsky)
(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.
Flags: needinfo?(g.maone)
(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?
Flags: needinfo?(g.maone)
(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?

Updated

4 years ago
Flags: needinfo?(g.maone)
(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.
Created attachment 829043 [details] [diff] [review]
Part 13 - New domain policy API. v2

Updated to the new design for the domain policy.
Attachment #828472 - Attachment is obsolete: true
Attachment #829043 - Flags: review?(bzbarsky)
Created attachment 829044 [details] [diff] [review]
Part 14 - Tests. v2
Attachment #828473 - Attachment is obsolete: true
Attachment #829044 - Flags: review?(bzbarsky)
(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.
(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.
(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
(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.
(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
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.
Attachment #828459 - Flags: review?(bzbarsky) → review+
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...
Attachment #828460 - Flags: review?(bzbarsky) → review+
Comment on attachment 828461 [details] [diff] [review]
Part 3 - Remove CheckFunctionAccess. v1

r=hurray!
Attachment #828461 - Flags: review?(bzbarsky) → review+
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?
Flags: needinfo?(bobbyholley+bmo)
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
Attachment #828463 - Flags: review?(bzbarsky) → review+
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
Attachment #828464 - Flags: review?(bzbarsky) → review+
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
Attachment #828465 - Flags: review?(bzbarsky) → review+
(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.
Flags: needinfo?(bobbyholley+bmo)
Attachment #828466 - Flags: feedback?(odvarko)
(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).
Flags: needinfo?(bzbarsky)
(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.
> 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.
Flags: needinfo?(bzbarsky)
> 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.
(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.
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.
Attachment #828462 - Flags: review?(bzbarsky) → review+
(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.
> 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.
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.
Attachment #828466 - Flags: review?(bzbarsky) → review+
Comment on attachment 828467 [details] [diff] [review]
Part 9 - Move nsXULDocument to new-style scripts checks. v1

r=me
Attachment #828467 - Flags: review?(bzbarsky) → review+
Comment on attachment 828468 [details] [diff] [review]
Part 10 - Remove nsIScriptContext script check in nsScriptLoader. v1

r=me
Attachment #828468 - Flags: review?(bzbarsky) → review+
Comment on attachment 828470 [details] [diff] [review]
Part 11 - Remove per-JSContext script toggling. v1

r=me, and good riddance.  ;)
Attachment #828470 - Flags: review?(bzbarsky) → review+
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.
Attachment #828471 - Flags: review?(bzbarsky) → review+
Created attachment 829644 [details] [diff] [review]
rollup.patch
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+.
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.
Attachment #829044 - Flags: review?(bzbarsky) → review+
Created attachment 829658 [details] [diff] [review]
Part 13 - New domain policy API. v3

Now with DomainPolicy.h.
Attachment #829043 - Attachment is obsolete: true
Attachment #829043 - Flags: review?(bzbarsky)
Attachment #829658 - Flags: review?(bzbarsky)
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.
Comment on attachment 829658 [details] [diff] [review]
Part 13 - New domain policy API. v3

r=me
Attachment #829658 - Flags: review?(bzbarsky) → review+
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;;

;;
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
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?
(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.
https://tbpl.mozilla.org/?tree=Try&rev=c99c82511097
Green.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=cd0e2e0ef136
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.
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Duplicate of this bug: 807371

Updated

4 years ago
Blocks: 939135
Depends on: 941707

Updated

4 years ago
Depends on: 944407
Depends on: 953344

Comment 84

3 years ago
Is there going to be documentation available for the new API before it hits the release channel? If so, where?
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.
Flags: needinfo?(bobbyholley+bmo)
Keywords: dev-doc-needed
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.
Flags: needinfo?(bobbyholley+bmo)
Depends on: 972478

Updated

3 years ago
Depends on: 986542

Updated

3 years ago
Depends on: 986886
Depends on: 998456
You need to log in before you can comment on or make changes to this bug.