Last Comment Bug 671148 - test_evalInSandbox.xul appears to be partly bogus
: test_evalInSandbox.xul appears to be partly bogus
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla9
Assigned To: Blake Kaplan (:mrbkap)
:
:
Mentors:
Depends on:
Blocks: sync-about-blank
  Show dependency treegraph
 
Reported: 2011-07-12 17:55 PDT by Blake Kaplan (:mrbkap)
Modified: 2011-08-31 02:12 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix (2.20 KB, patch)
2011-07-12 17:55 PDT, Blake Kaplan (:mrbkap)
peterv: review-
Details | Diff | Splinter Review
Proposed fix (3.04 KB, patch)
2011-08-16 15:13 PDT, Blake Kaplan (:mrbkap)
peterv: review+
Details | Diff | Splinter Review

Description Blake Kaplan (:mrbkap) 2011-07-12 17:55:09 PDT
Created attachment 545554 [details] [diff] [review]
Proposed fix

In bug 533596, comment 26, Henri pointed to js/src/xpconnect/tests/chrome/test_evalInSandbox.xul (note that the bug number in the test is actually bogus) and asks why it fails with his patch for bug 543435. I debugged it today and found that the test is (partly bogus).

The part of the test that's failing appears to want to test a chrome sandbox. The way it does that is to load a data: URL in an iframe in a chrome document. Unfortunately, it appears that there's a copy/paste error in the test which makes the iframe type=content. This doesn't appear to affect current trunk.

However, even when I make the iframe type=chrome, the test fails with the patch for bug 543435. Henri, I think the remainder of the fix is figuring out why your changes leave the iframe with a content principal. Either way, we should probably take this fix.

As a note, if it's too hard to fix the data: URL behavior, we could also change the test to use an explicit chrome: URL.
Comment 1 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-07-13 05:56:56 PDT
(In reply to comment #0)
> Created attachment 545554 [details] [diff] [review] [review]
> Proposed fix
> 
> In bug 533596, comment 26, Henri pointed to
> js/src/xpconnect/tests/chrome/test_evalInSandbox.xul (note that the bug
> number in the test is actually bogus) and asks why it fails with his patch
> for bug 543435. I debugged it today and found that the test is (partly
> bogus).

Thank you!

> However, even when I make the iframe type=chrome, the test fails with the
> patch for bug 543435. Henri, I think the remainder of the fix is figuring
> out why your changes leave the iframe with a content principal. Either way,
> we should probably take this fix.
> 
> As a note, if it's too hard to fix the data: URL behavior, we could also
> change the test to use an explicit chrome: URL.

I'm not at all familiar with what *should* be happening. Do you mean that a data: URL should load with a chrome principal but the initial about:blank interferes with the principal selection?
Comment 2 Henri Sivonen (:hsivonen) (Not reading bugmail or doing reviews until 2016-10-03) 2011-07-18 04:02:48 PDT
> +  <iframe type="chrome"

Are you sure that type="chrome" has an effect on HTML iframes? smaug told me on IRC that it doesn't.
Comment 3 Blake Kaplan (:mrbkap) 2011-07-22 11:59:54 PDT
Oh, that's a good point. But I think HTML iframes always inherit the principal of their owner document, in which case the data: URL should have chrome privileges anyway, right?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-07-22 20:50:33 PDT
HTML iframes always inherit their parent docshell type.  So those iframes have the same type of docshell as the parent page loading them.

However that page has a _content_ type docshell, because this is a browser-chrome test and those are run in a content-type docshell (which sucks and all, but there we are).  I was pretty sure we had a bug on that, but I can't find it right now; there are workarounds for it all over the docshell chrome tests, opening new windows which actually have chrome-type docshells to run the test in....

So in this case we have a situation where the loadinfo passed to the docshell has the system principal (the NodePrincipal() of that <iframe>) as the owner, but the load is happening in a content docshell.

Now we get to the code at <http://hg.mozilla.org/mozilla-central/file/887fad3ebc0b/docshell/base/nsDocShell.cpp#l1405>.  ownerPrincipal is system, we're a typeContent docshell, so we null out the owner and set inheritOwner to true.  That means we pass INTERNAL_LOAD_FLAGS_INHERIT_OWNER to InternalLoad.

We're loading a data: URI, which inherits the security context, so this actually has an effect we end up calling GetInheritedPrincipal(PR_TRUE) at http://hg.mozilla.org/mozilla-central/file/887fad3ebc0b/docshell/base/nsDocShell.cpp#l8178

Now GetInheritedPrincipal(PR_TRUE) means we use the principal of the currently loaded document, if any.  If there isn't one, we us the principal of our same-type parent, if any.  If there isn't one, we just go ahead and EnsureContentViewer and use the principal of the resulting document.

Now before Henri's change, I bet we did NOT have a current document, and our parent docshell is same-type, so we used the parent document's principal, which is system.  But with Henri's change we always have an about:blank in the docshell, so we inherit whatever principal that has.

Looking at the patch in bug 543435, that will be a null principal due to the code added to EnsureContentViewer.  That's the right behavior if we want the synthetic about:blank match the behavior of actual about:blank loads; that's why that hunk was added.  But it breaks this particular case.  Note that four required parts of the breakage are that (1) a system-principal page (2) loaded in a content-type docshell has a subframe (3) whose initial src value (4) points to a javascript: or data: URI,

I believe the test in question will fail even without Henri's patch if it's changed to wait until after the initial about:blank loads in the subframe before doing the data: load.  Worth verifying.

Fixing the test is straightforward: it needs to open a new chrome-type window to run itself in, or use a chrome:// URI, or whatever.

An interesting question is how we feel about the compat impact of this on the various abusers who're loading system-principal documents in content-type docshells.... :(
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-07-22 20:51:40 PDT
One option is to make the "inherit from current document" case of GetInheritedPrincipal initially pretend there's no current document if the current document is the initial about:blank....
Comment 6 Peter Van der Beken [:peterv] 2011-07-25 01:45:27 PDT
Comment on attachment 545554 [details] [diff] [review]
Proposed fix

Blake, I'm assuming we don't want this in light of comment 4. Rerequest review if I'm mistaken.
Comment 7 Blake Kaplan (:mrbkap) 2011-08-16 15:13:49 PDT
Created attachment 553600 [details] [diff] [review]
Proposed fix
Comment 8 Marco Bonardo [::mak] 2011-08-31 02:12:45 PDT
http://hg.mozilla.org/mozilla-central/rev/df5afd3a325d

Note You need to log in before you can comment on or make changes to this bug.