Closed
Bug 671148
Opened 13 years ago
Closed 13 years ago
test_evalInSandbox.xul appears to be partly bogus
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: mrbkap, Assigned: mrbkap)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
3.04 KB,
patch
|
peterv
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #545554 -
Flags: review?(peterv)
(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?
> + <iframe type="chrome"
Are you sure that type="chrome" has an effect on HTML iframes? smaug told me on IRC that it doesn't.
Assignee | ||
Comment 3•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
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.
Attachment #545554 -
Flags: review?(peterv) → review-
Assignee | ||
Comment 7•13 years ago
|
||
Assignee: nobody → mrbkap
Attachment #545554 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #553600 -
Flags: review?(peterv)
Updated•13 years ago
|
Attachment #553600 -
Flags: review?(peterv) → review+
Comment 8•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in
before you can comment on or make changes to this bug.
Description
•