495337 - Make sessionStorage use principals instead of string domains.

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

This is a followup to bug 455070, which was partially backed out in bug 495112 because it casused bug 494502 and bug 494805. The change in bug 455070 that caused the problems was to make sessionStorage compare principals instead of string origins, so this bug is about fixing the that remaining aspect of bug 455070.

Comment 1

[Honza Bambas (:mayhemer)]

You wanted to say "compare principals instead of string domains". And it mainly caused bug 494543 (and its duplicate bug 494810).

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Since we need to serialize the sessionStorage objects (as part of session restore), I wonder if the correct thing to do is actually to use strings, but store domain+scheme+port instead of just domain. And definitely use principal->GetURI, not principal->GetDomain!

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1

This is more a preview of my approach to fix this. I do subsumes on subject's principal and not on the principal we demand the storage for, the same is done when accessing sessionStorage attributes. When Subsumes fails I do security check of the URIs to bypass the domain change. We could add call to piStorage->GetPrincipal()->Equals(aPrincipal); to check we really provide storage to the correct principal. But then when it fails we have do security check of the uris as well. This doesn't introduce the session store regression, no need for patch for bug 494543.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So what exactly is the point of the CanAccess check in nsDocShell::GetSessionStorageForPrincipal?  Why do we care what the subject is?

Why is |origin| in that function not nsXPIDLCString?

Why are you checking URIs if Subsumes() returned error?  Error there should mean you return false, in my opinion.

This patch makes session storage from signed jars accessible to unsigned pages on the same host and vice versa.  Is that desirable?  It can also lead to weird results if we introduce new classes of principals, because you're making assumptions about how GetURI on two principals should relate them.  It might make more sense to have versions of Subsumes() and Equals() that ignore document.domain...

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to comment #4)
> So what exactly is the point of the CanAccess check in
> nsDocShell::GetSessionStorageForPrincipal?  Why do we care what the subject is?
> 

We do care because we don't want to allow a page access the sessionStorage object of a page with a different origin, primarily, and allow access to any page's sessionStorage for system principal. The approach in the patch fixes the session restore problem, bug 494543.

> Why is |origin| in that function not nsXPIDLCString?
> 

Fixed in upcoming patch.

> Why are you checking URIs if Subsumes() returned error?  Error there should
> mean you return false, in my opinion.
> 
> This patch makes session storage from signed jars accessible to unsigned pages
> on the same host and vice versa.  Is that desirable?  It can also lead to weird
> results if we introduce new classes of principals, because you're making
> assumptions about how GetURI on two principals should relate them.  It might
> make more sense to have versions of Subsumes() and Equals() that ignore
> document.domain...

As you suggested, I changed nsIPrincipal interface to have two more methods: subsumesIgnoreDomain and equalsIgnoreDomain. Doesn't need more explanation.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v2

Comment 7

[Boris Zbarsky [:bzbarsky]]

> We do care because we don't want to allow a page access the sessionStorage
> object of a page with a different origin, primarily, and allow access to any
> page's sessionStorage for system principal

That doesn't answer my question.  When that code was written, last I looked at it, it was a belt-and-suspenders check.  That's why it asserted if it failed!  In other words, it was just verifying that some other code has functioned correctly and prevented access if it needs to be prevented.  It was doing so because we did not fully trust the other code and were rushing things in for reasons that I think were stupid.

So the questions that I have, and that I keep asking (though with the assumption that you know what the code was doing and what GetSubjectPrincipal does, which might be a wrong assumption), and that you never answer are:

1) Did something about the other check change so that we trust it even less? 
   If not, should we just be removing this check and making sure the other
   check actually works?
2) If we think a check here is required, are we sure that all C++ callers of
   this API really act on behalf of the subject principal?

Basically, any GetSubjectPrincipal call is an automatic red flag for me unless it's happening _very_ close to the place where we'd enter code from JS.  Ideally in the JS-to-C++ translation layer.  Any time we have it in code callable from C++, we have the potential for really nasty bugs.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to comment #7)
> > We do care because we don't want to allow a page access the sessionStorage
> > object of a page with a different origin, primarily, and allow access to any
> > page's sessionStorage for system principal
> 
> That doesn't answer my question.  When that code was written, last I looked at
> it, it was a belt-and-suspenders check.  That's why it asserted if it failed! 
> In other words, it was just verifying that some other code has functioned
> correctly and prevented access if it needs to be prevented.  It was doing so
> because we did not fully trust the other code and were rushing things in for
> reasons that I think were stupid.
> 
> So the questions that I have, and that I keep asking (though with the
> assumption that you know what the code was doing and what GetSubjectPrincipal
> does, which might be a wrong assumption), and that you never answer are:

True is that my knowledge of this is not that deep.

> 
> 1) Did something about the other check change so that we trust it even less?

If "the other check" == "check when accessing properties" then NO. 
 
>    If not, should we just be removing this check and making sure the other
>    check actually works?

We should remove it (as you propose it). The other check is sufficient.

> 2) If we think a check here is required, are we sure that all C++ callers of
>    this API really act on behalf of the subject principal?

From bug 458091 comment 27 by Dave Camp:

'I added the CanAccess() assertion because I wanted it to be a bit more clear
that there are implicit security implications of that mStorages hash key. 
There should be no case where CanAccess() returns false, because the hash key
should be unique to the set of people that can access that storage.'

So, I propose the other suggestion of mine, which is to extract the existing sessionStorage principal and do EqualsIgnoreDomain check against the principal we demand the storage for.

> 
> Basically, any GetSubjectPrincipal call is an automatic red flag for me unless
> it's happening _very_ close to the place where we'd enter code from JS. 
> Ideally in the JS-to-C++ translation layer.  Any time we have it in code
> callable from C++, we have the potential for really nasty bugs.

Could you lead me please to some documentation or examples to get better understanding for next time?

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v3

As per comment 8.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Could you lead me please to some documentation or examples to get better
> understanding for next time?

Understanding of what?  GetSubjectPrincipal assumes that the most-recently-called JS function represents "who" is doing the action.  Any time that assumption is false, you get problems.

Looking at the patch now.

Comment 11

[Boris Zbarsky [:bzbarsky]]

>+++ b/caps/src/nsNullPrincipal.cpp
>+nsNullPrincipal::SubsumesIgnoreDomain(nsIPrincipal *aOther, PRBool *aResult)
>+{
>+  return SubsumesIgnoreDomain(aOther, aResult);

Is someone has the misfortune to call this function, it'll blow out the stack and crash.  Should that be a Subsumes() call?

>+++ b/docshell/base/nsDocShell.cpp
>-GetPrincipalDomain(nsIPrincipal* aPrincipal, nsACString& aDomain)
...
>-  rv = innerURI->GetAsciiHost(aDomain);

>@@ -2156,55 +2128,60 @@ nsDocShell::GetSessionStorageForPrincipa
>-    rv = GetPrincipalDomain(aPrincipal, currentDomain);
...
>+    rv = aPrincipal->GetOrigin(getter_Copies(origin));

So why is the change from asciiHost to host ok here?  I'd think this would cause a problem (and that we should have tests to catch this problem).  Same at the other place where you make this change.

>-        PRBool canAccess = piStorage->CanAccess(aPrincipal);
...
>+        nsresult rv = aPrincipal->EqualsIgnoreDomain(storagePrincipal, &equals);

So this is switching from a Subsumes check to an Equals check, right?  Why?

>+++ b/dom/src/storage/nsDOMStorage.cpp
>-  mStorage->mSecurityChecker = mStorage;
>+  mStorage->mSecurityChecker = this;

I'm not sure what the implications of this change are....

And do we sometimes set mStorage->mSecurityChecker to something other than |this|?

Comment 12

[Honza Bambas (:mayhemer)]

Sorry for late answer, I lost my paper todo-list...

(In reply to comment #11)
> >+++ b/caps/src/nsNullPrincipal.cpp
> >+nsNullPrincipal::SubsumesIgnoreDomain(nsIPrincipal *aOther, PRBool *aResult)
> >+{
> >+  return SubsumesIgnoreDomain(aOther, aResult);
> 
> Is someone has the misfortune to call this function, it'll blow out the stack
> and crash.  Should that be a Subsumes() call?

Thanks for the catch.

> 
> >+++ b/docshell/base/nsDocShell.cpp
> >-GetPrincipalDomain(nsIPrincipal* aPrincipal, nsACString& aDomain)
> ...
> >-  rv = innerURI->GetAsciiHost(aDomain);
> 
> >@@ -2156,55 +2128,60 @@ nsDocShell::GetSessionStorageForPrincipa
> >-    rv = GetPrincipalDomain(aPrincipal, currentDomain);
> ...
> >+    rv = aPrincipal->GetOrigin(getter_Copies(origin));
> 
> So why is the change from asciiHost to host ok here?  I'd think this would
> cause a problem (and that we should have tests to catch this problem).  Same at
> the other place where you make this change.

Good question. Should we have something like a new method nsIPrincipal.asciiOrigin ?

> 
> >-        PRBool canAccess = piStorage->CanAccess(aPrincipal);
> ...
> >+        nsresult rv = aPrincipal->EqualsIgnoreDomain(storagePrincipal, &equals);
> 
> So this is switching from a Subsumes check to an Equals check, right?  Why?

Yes, exactly to EqualsIgnoreDomain. The reason is actually described here (see also comment 8)

From bug 458091 comment 27 by Dave Camp:

'I added the CanAccess() assertion [now altered directly by Equals()] because I wanted it to be a bit more clear
that there are implicit security implications of that mStorages hash key [now origin string]. 
There should be no case where CanAccess() [now Equals()] returns false, because the hash key
should be unique to the set of people that can access that storage.'

The origin string used to map items in the hash table is an implicit security check. This check is double checked by checking the principal a storage was demanded for really is the principal for which that storage was originally created. Originally, the check was hidden in the CanAccess method but it's implementation has changed.

> 
> >+++ b/dom/src/storage/nsDOMStorage.cpp
> >-  mStorage->mSecurityChecker = mStorage;
> >+  mStorage->mSecurityChecker = this;
> 
> I'm not sure what the implications of this change are....
> 

It reverts security checking to nsDOMStorage2 implementation. See bug 494805 to explain the mSecurityChecker member and bug 495112 why it was assigned mStorage and not |this|. In a nut shell: mSecurityChecker is there because nsDOMStorage2 wraps nsDOMStorage that is doing all security checks. But nsDOMStorage2 implements CanAccess method a different way. mSecurityChecker allows nsDOMStorage to get knowledge of nsDOMStorage2 implementation (through nsPIDOMStorage interface).

> And do we sometimes set mStorage->mSecurityChecker to something other than
> |this|?

When nsDOMStorage is initiated as globalStorage, mSecurityChecker is assign nsDOMStorage instance (not nsDOMStorage2) as it is using mapping and security check per domain, not per origin; it actually preserve the original implementation before localStorage has been introduced.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Should we have something like a new method nsIPrincipal.asciiOrigin

I would support that.  Or even making GetOrigin ASCII...  Check with dveditz?

> The origin string used to map items in the hash table is an implicit security
> check. This check is double checked by checking the principal a storage was
> demanded for really is the principal for which that storage was originally
> created.

Aha!  Can we add that as a comment in the code?

Thanks for the clarification on mSecurityChecker.

Comment 14

[Johnny Stenback (:jst)]

Not blocking on this, but if we get a patch we should consider taking it for 1.9.2 IMO.

Comment 15

[Daniel Veditz [:dveditz]]

Rather than making a new asciiOrigin, is there a problem if we just make GetOrigin() return ascii? It's [noscript] so addons aren't using it, and I don't see any callers in our code either except in nsPrincipal.cpp itself. Which seems odd, I'm sure there used to be some.

if nsIPrincipal is unfrozen, would it make more sense to add an argument to subsumes and equals than to add two completely new (and ugly named) methods?

Comment 16

[Honza Bambas (:mayhemer)]

We should fix this bug in one of next major releases after Firefox 4.  Also needs feedback from web developers.

Comment 17

[Michal Zalewski]

Hi folks,

Note that the current behavior is not just a spec conformance issue, but also a security problem.

Specifically, you currently permit HTTP pages to inject properties into HTTPS contexts if they are first to the party (HTTPS -> HTTP throws an exception); and to add insult to injury, if these are later modified in the HTTPS context, they remain accessible to HTTP.

This is a very big deal for HTTPS-only applications that want to resist MITM attackers on open wifi, etc.

Comment 18

[Johnny Stenback (:jst)]

Honza, are you still planning to continue this work?

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #18)
> Honza, are you still planning to continue this work?

Last time we broke a lot of sites.  There were voices to leave the code as it because many sites use document.domain and interchange values between http and https.  We can reopen the discussion and get some consensus.  From my modest point of view with experience in evangelism I don't expect it to happen.

So we probably may close this bug as WONTFIX.  Maybe there will be negative response to that :)

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not sure I understand what the implications of this bug are.

The sessionStorage object is specific to a given webpage and it's siblings in the back/forward history, right? How it behaves should be irrelevant of changes to document.domain.

Comment 21

[Adam Barth]

This bug is a vulnerability in sessionStorage because it allows active network attackers to compromise the sessionStorage used by HTTPS sites..  Other browsers (for example, those that use WebKit) don't have this vulnerability.  The longer Firefox waits to fix this vulnerability, the more compatibility pain there will be.

Comment 22

[Honza Bambas (:mayhemer)]

As maintainer of the code I will check what other browsers do.  This bug is also about violating the HTML5 spec, AFAIK.

Comment 23

[Adam Barth]

Correct.  By violating the spec, you've created a security vulnerability.  I can re-file the issue under Security if that would be helpful.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

i'm still not understanding the behavioral difference here. Would love to have that written up in a comment we can point to in the whiteboard.

Comment 25

[Adam Barth]

Here's the exploit:

Setup: An HTTPS site (say https://example.org/) uses sessionStorage and wishes that data to have confidentiality and integrity from active network attackers.

1) The user visits an HTTP web site in the presence of an active network attacker.
2) The active network attacker intercepts that request and redirects it to http://example.org/.
3) The active network attacker intercepts that request and spoofs the following response:

<script>
document.domain = 'example.org';
sessionStorage.setItem('foo', 'haxored');
</script>

4) The integrity of the sessionStorage at https://example.org has now been compromised.  For example, consider the page https://example.org/alert-foo.html:

<script>
alert(sessionStorage.getItem('foo'));
</script>

This page will alert "haxored" even though that string was provided by an active network attacker.

This attack does not work in Chrome, Safari, or Opera; I haven't tested IE.  The HTML5 specification forbids this behavior.

Comment 26

[Adam Barth]

I mentioned this bug in a recent blog post, which gives some perspective for why I think it's important to fix this bug:

http://www.schemehostport.com/2011/10/integrity-for-sessionstorage.html

Comment 27

[Honza Bambas (:mayhemer)]

The record of site-break bugs:
- bug 494543 (dell.com) because it changes document.domain and principal check doesn't pass in our session store code ; it has been decided to *not* map sessionStorage by the effective origin in bug 494799 comment 11
- bug 494723 (tvguide.com) the same issue as on dell.com
- bug 494810 ; unfortunately I didn't make a note how to reproduce that bug

See a simple test case to reproduce the document.domain issue: bug 494799 comment 3 and 4.

I still need to figure out my note from bug 494543 comment 36, the https pages issue.

Other step is to check the test case with other browsers.

Comment 28

[Honza Bambas (:mayhemer)]

Note: after bug 687579 gets fixed, we should be able to remove nsIDOMStorageObsolete/nsDOMStorage interface and class pair along with fix for this bug.

Comment 29

[Adam Barth]

Do these sites work in other browsers?  My understanding is that all the other browsers follow the specification in this regard.

Comment 30

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Honza, are you intending to fix this so that we will be protected from the attack Adam demonstrated in comment 25? If so, we should file Tech Evangelism bugs ASAP against the major sites (dell.com, tvguide.com) that we know will break, so they can fix their sites before our fix reaches our users. If you think that the type of fix that Adam is suggesting is not right, then we should have a discussion about that right away with the security team, Jonas, and Adam so that we can find some a way to address the security issue and change the spec accordingly.

Comment 31

[Ben Adida [:benadida]]

I don't mean to pile on here, but I think Adam's point is particularly important. 

There are some gnarly security issues related to the lack of HTTPS cookie integrity against a network attacker, but it's too late to do anything about that because that misuses is widespread. We should fix this HTTPS/HTTP "leak" in localStorage before we have a more serious legacy problem, and we don't lose anything since any site that depends on this incorrect behavior will break on other browsers.

Comment 32

[Honza Bambas (:mayhemer)]

BTW: localStorage is not affected by this bug as it is mapped by origin (according the spec).  This bug is strictly a sessionStorage issue.

Comment 33

[Honza Bambas (:mayhemer)]

I did some basic tests:

Test 1:
- navigate to sub.test.domain
- set a value of a key
- change document.domain to test.domain (a level up)
- read the value back

Chrome (current): the value is the one previously stored
IE 9: the same behavior as Chrome
Firefox Nightly: the value is 'null'

This happens because we create a whole new instance of window.sessionStorage after document.domain has changed - storage->CanAccess returns false (domains no longer equal) for the current window's principal at [1] and mSessionStorage is then nullified and recreated.  There is no exception threw.


Test 2:
- navigate to sub.test.domain
- change document.domain to test.domain (a level up)
- set a value of a key
- navigate to a domain one level up (test.domain) with an anchor in the document
- read the value back

Chrome (current): the value is 'undefined'
IE 9: the same behavior as Chrome
Firefox Nightly: the value is the one set at the third step of the test


Test 3 (most serious):
- navigate to sub.test.domain
- set a value
- navigate to https://sub.test.domain with an anchor in the document
- read the value back

Chrome (current): the value is 'undefined'
IE 9: the same behavior as Chrome
Firefox Nightly: the value is the one set at the second step of the test -> there is a way to inject data


Test 4:
- navigate to https://sub.test.domain
- set a value
- navigate to sub.test.domain (non-secured) with an anchor in the document
- read the value back

Chrome (current): the value is 'undefined'
IE 9: the same behavior as Chrome
Firefox Nightly: the value is 'null' and cannot be modified (we throw a security exception on attempt to that)


All tests show we are probably the last browser that behaves against the spec, we really overslept here!  I will now check with Chrome and IE on the regression bugs and if there are no problems (what is very likely), I will test with the missing parts of the fix for bug 455070.  If all goes well, we can land it (again).


[1] http://hg.mozilla.org/mozilla-central/annotate/349f3d4b2d87/dom/base/nsGlobalWindow.cpp#l8112

Comment 34

[Honza Bambas (:mayhemer)]

I have pushed an updated patch v3 from this bug to the try server.  If all goes well I polish it and submit for a new review round.

Testing with dell.com and tvguide works, but both those sites have evidently changed a lot, so it is hardly a valid test.

Comment 35

[Honza Bambas (:mayhemer)]

So, I am running a newer version of the patch on the try server.

It runs to a following problems:

This changeset https://hg.mozilla.org/try/rev/c434104cded6 (adding only a new method to nsIPrincipal interface, never used anywhere) is causing failures on try server: https://tbpl.mozilla.org/?tree=Try&rev=575957dff664 - breaks Jetpack tests build.  Not sure why.

This changeset https://hg.mozilla.org/try/rev/aaf9475a4e01 (the whole patch) also breaks R(J) - JavaScript reftests: https://tbpl.mozilla.org/?tree=Try&rev=4bd1ae1bb695, bugs/684893-happy-eyeballs-2.patch
bugs/timestamp-simplified.patch

I am not sure what the problem is, any ideas?

Comment 36

[Boris Zbarsky [:bzbarsky]]

Changing the nsIPrincipal IID can at least cause session restore issues....

Comment 37

[Josh Aas]

Honza - can you post your most recent patch even if it doesn't pass try? Would be good to have the current state of the work available.

Comment 38

[Honza Bambas (:mayhemer)]

Attachment: latest version breaking try build

I plan to finish this.  I wasn't able to figure out from the logs what the cause of red (not just orange) was.  Maybe trying locally may help, also worth to push this again to try.

Comment 39

[Honza Bambas (:mayhemer)]

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

So, I'm getting somewhere, failing jsreftest is conditioned as:

fails-if(!xulRuntime.shell) script forin-002.js # bug - NS_ERROR_DOM_NOT_SUPPORTED_ERR line 112

When I remove the condition the test passes as expected.


I have no idea what the condition really means.

Adding dbaron and jesse to ask for help with this, since they introduced the change.

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I believe jsreftests can be run with either the reftest harness or with a JS shell-based harness.

Comment 41

[Jesse Ruderman]

I don't remember touching jstests.list or forin-002.js. Looks more like bc and dmandelin, and both of them neglected to add a bug number :(

http://hg.mozilla.org/mozilla-central/diff/4b1f9afcedb1/js/src/tests/ecma_2/Statements/jstests.list

Your try log shows a TEST-UNEXPECTED-PASS.  So you're unexpectedly changing the behavior of the test when the test is run in the browser.  Please make a reduced testcase and understand why your patch changes the behavior.

Comment 42

[Honza Bambas (:mayhemer)]

(In reply to Jesse Ruderman from comment #41)
> I don't remember touching jstests.list or forin-002.js. Looks more like bc
> and dmandelin, and both of them neglected to add a bug number :(
> 

Check bug 509269.

> http://hg.mozilla.org/mozilla-central/diff/4b1f9afcedb1/js/src/tests/ecma_2/
> Statements/jstests.list
> 
> Your try log shows a TEST-UNEXPECTED-PASS.  So you're unexpectedly changing
> the behavior of the test when the test is run in the browser.  Please make a
> reduced testcase and understand why your patch changes the behavior.

Can you please tell me what is the condition "xulRuntime.shell" ?  I strongly believe my patch is changing this condition and not the test behavior it self.

Thanks.

Comment 43

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #42)
> I strongly believe my patch is changing this condition and not the test
> behavior it self.

To confirm this, I just tried to add the condition fails-if(!xulRuntime.shell) to a different random test and I was getting the same TEST-UNEXPECTED-PASS results.

Comment 44

[Jesse Ruderman]

(In reply to Honza Bambas (:mayhemer) from comment #42)
> (In reply to Jesse Ruderman from comment #41)
> > I don't remember touching jstests.list or forin-002.js. Looks more like bc
> > and dmandelin, and both of them neglected to add a bug number :(
> > 
> 
> Check bug 509269.

I don't see what that has to do with jstests.list, forin-002.js, or the conditions in question.

> Can you please tell me what is the condition "xulRuntime.shell" ?

The condition xulRuntime.shell means "the test is running in the shell rather than the browser".

fails-if(!xulRuntime.shell) means "I expect this test to fail when run in the browser, and pass when run in the shell".

> I strongly believe my patch is changing this condition and not the test
> behavior it self.

Why do you believe that?

> To confirm this, I just tried to add the condition
> fails-if(!xulRuntime.shell) to a different random test and I was getting the
> same TEST-UNEXPECTED-PASS results.

I don't see how that follows. If you tell the harness to expect a failure, and the test continues to pass, of course it's going to complain about an unexpected pass.

Comment 45

[Bob Clary [:bc] (inactive)]

You can run the test individually in the browser by loading (adjust path to suit):

file:///work/mozilla/builds/nightly/mozilla/js/src/tests/jsreftest.html?test=file:///work/mozilla/builds/nightly/mozilla/js/src/tests/ecma_2%2FStatements%2Fforin-002.js

You'll need to either user a profile with the user.js from the js/src/tests directory or adjust the preferences to allow UniversalXPConnect from the file://

To run a really really old version of the js test suite in the browser you can load:

http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.html?test=ecma_2%2FStatements%2Fforin-002.js;language=type;text/javascript

I think the issue in the browser is related to the differences between loading the tests via http: and via the local file:

Comment 46

[Honza Bambas (:mayhemer)]

(In reply to Jesse Ruderman from comment #44)
> (In reply to Honza Bambas (:mayhemer) from comment #42)
> > (In reply to Jesse Ruderman from comment #41)
> > > I don't remember touching jstests.list or forin-002.js. Looks more like bc
> > > and dmandelin, and both of them neglected to add a bug number :(
> > > 
> > 
> > Check bug 509269.
> 
> I don't see what that has to do with jstests.list, forin-002.js, or the
> conditions in question.

Looks like the bug reference in the changeset is wrong, sorry:
http://hg.mozilla.org/mozilla-central/rev/4b1f9afcedb1

> 
> > Can you please tell me what is the condition "xulRuntime.shell" ?
> 
> The condition xulRuntime.shell means "the test is running in the shell
> rather than the browser".
> 
> fails-if(!xulRuntime.shell) means "I expect this test to fail when run in
> the browser, and pass when run in the shell".

Thanks for this explanation.

> 
> > I strongly believe my patch is changing this condition and not the test
> > behavior it self.
> 
> Why do you believe that?

Because my patch from this bug causes to fail tests that are protected by that condition only.

I still don't know, whether |xulRuntime.shell| value changes with my patch or whether the test is what is actually affected.

To figure this out is my next step.

> 
> > To confirm this, I just tried to add the condition
> > fails-if(!xulRuntime.shell) to a different random test and I was getting the
> > same TEST-UNEXPECTED-PASS results.
> 
> I don't see how that follows. If you tell the harness to expect a failure,
> and the test continues to pass, of course it's going to complain about an
> unexpected pass.

I'm trying to understand how the test harness works here.

Maybe, I'm breaking the same thing the tests had been changed for in http://hg.mozilla.org/mozilla-central/rev/4b1f9afcedb1.  Only thing I seem to modify that could influence the tests is the change to nsIPrincipal interface.

(In reply to Bob Clary [:bc:] from comment #45)
> You can run the test individually in the browser by loading (adjust path to
> suit):
> 
> file:///work/mozilla/builds/nightly/mozilla/js/src/tests/jsreftest.
> html?test=file:///work/mozilla/builds/nightly/mozilla/js/src/tests/
> ecma_2%2FStatements%2Fforin-002.js
> 
> You'll need to either user a profile with the user.js from the js/src/tests
> directory or adjust the preferences to allow UniversalXPConnect from the
> file://
> 
> To run a really really old version of the js test suite in the browser you
> can load:
> 
> http://test.bclary.com/tests/mozilla.org/js/js-test-driver-standards.
> html?test=ecma_2%2FStatements%2Fforin-002.js;language=type;text/javascript
> 
> I think the issue in the browser is related to the differences between
> loading the tests via http: and via the local file:

I can reproduce the test failure locally with python reftest/runreftest.py --appname=../dist/bin/firefox.exe --utility-path=bin --extra-profile-file=bin/plugins --extra-profile-file=jsreftest/tests/us
er.js jsreftest/tests/ecma_2/Statements/jstests.list

(I've extracted the tests to a dir under _obj/_tests/)

Comment 47

[Honza Bambas (:mayhemer)]

The condition is not changed at all.

The "problem" here is, that I'm actually fixing the original reason the tests had been turned to known-failing.

forin-002.js fails on line 87: object[property] == eval(property).

This line (no idea why) invokes windows.sessionStorage.  W/o the patch it fails since there is no domain to bound the storage to.  With the patch, we have a valid principal and get the mapping key to bound the storage to, so we are not failing to get the storage.  The origin key is "file:///.../forin-002.js" what is expected.

So, reverting some of http://hg.mozilla.org/mozilla-central/rev/4b1f9afcedb1 is the solution here.

I will update the patch.

Comment 48

[Honza Bambas (:mayhemer)]

Attachment: v4

Boris, should be updated according you review comments from comment 11.

Bob, please check the changes in jsreftest list files, here is a try run:

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

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 599598 [details] [diff] [review]
v4

This patch needs a real commit comment.  Please fix that.

You have a PR_TRUE in there.  Please use true instead.

r=me with those two nits.

Comment 50

[Honza Bambas (:mayhemer)]

(In reply to Boris Zbarsky (:bz) from comment #49)
> Comment on attachment 599598 [details] [diff] [review]
> v4
>
> This patch needs a real commit comment.  Please fix that.
I always update just before commit my self.
> You have a PR_TRUE in there.  Please use true instead.
Ah, leftover got in again..
> r=me with those two nits.
Will be fixed before landing.  Thanks.

Comment 51

[Bob Clary [:bc] (inactive)]

Comment on attachment 599598 [details] [diff] [review]
v4

js tests look good and all js tests passed in the try.

Comment 52

[Honza Bambas (:mayhemer)]

Comment on attachment 599598 [details] [diff] [review]
v4

Thanks for the reviews.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8937b91b8435

Comment 53

[Honza Bambas (:mayhemer)]

PR_TRUE -> true fix.  Slippy little b*tch...
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc589c7cdc69

Comment 54

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/8937b91b8435
https://hg.mozilla.org/mozilla-central/rev/fc589c7cdc69

Comment 57

[Ben Bucksch (:BenB)]

See bug 562646: localStorage works in resource: URLs, but not in chrome: URLs.