1234021 - With "Accept cookies from sites" unchecked, history of back/forward buttons fails with "Error: SecurityError: The operation is insecure." in SessionStorage.jsm

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[TBeholder]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151210085006

Steps to reproduce:

1) open a new empty tab, or "open link in new tab" the following URL (this didn't happen when there's another site before it in tab history):
2) visit http://www.openxcom.com/
3) follow a link within the site, e.g. "mods"
4) right click on "Back" or "Forward" button

Whether NoScript is set to allow or deny does not matter, and the bug happens even when FF is started in Safe Mode.
Same happens with http://lparchive.org/X-COM-UFO-Defense/ - but not in Safe Mode, though NoScript status still doesn't matter.
This also doesn't happen when current or previous URL is HTTPS (though this may cause another bug, with History menu being obviously incorrect).


Actual results:

- nothing visible tab history menu does not appear
- console logs error:
Timestamp: ...
Error: SecurityError: The operation is insecure.
Source File: resource://app/modules/sessionstore/SessionStorage.jsm
Line: 148


Expected results:

context menu: tab history should pop up, like in any other tab.

Comment 1

[TBeholder]

I was not sure - it was sessionstore, but it throws SecurityError when calling something.

Anyway, it appears on different sites, but not every other, and repeatability varies by site. Like with lparchive.org being immune in Safe Mode, but affected in my normal setup (even though there are no unusual settings for it - no greasemonkey scripts, nothing, just one more site with cookies and scripts denied by default). Sometimes the bug disappears in some tab after some browsing around even on an affected site, but appears again in a new tab with the same site.

Aw, forgot to add ufopaedia.org to the list. =)

Also, it doesn't seem to matter whether an immune URL is in back or forward part of the history - it was enough to visit "about:mozilla" and go back to make history menu work in an affected tab.

Comment 2

[Elhem Enohpi]

I've reported a similar problem in bug 1238178, could you see if any of it applies to you? Eg., other problems like Undo Close Tab not working, pages not being restored, or closed windows being re-opened at restart.

I tested the URLs you listed, but only one of them, lparchive.org, caused the problems I described.

And to confirm, you have cookies disabled for these sites? That agrees with my testing.

But disabling javascript doesn't help? That's not what I'd expect. Could you perhaps try turning off javascript another way, eg. with the QuickJS addon or similar?

Comment 3

[Loic]

STR:
1) Uncheck the option "Accept cookies from sites"
2) Open a new tab and load http://www.openxcom.com/
3) Click on a menu entry like "MODS"
4) Right-click on back button to display history

Result:
No context menu with 2 entries like http://i.imgur.com/YoJtoLb.jpg for back button. Same for forward button.

Error: SecurityError: The operation is insecure.
Source File: resource://app/modules/sessionstore/SessionStorage.jsm
Line: 148

Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cadeefe3e92791a426c3e950b78e4a38a7093e9a&tochange=1056ac371c0468cde5600a7c5765dc170c5f77e5

Bobby Holley — Bug 1201747 - Don't inspect the subject principal in StorageAllowedForPrincipal. r=mystor

Comment 4

[Bobby Holley (:bholley)]

Bug 1201747 was just fixing a mistake in bug 1184973, so given the regression range, that's the real culprit here.

Comment 5

[Nika Layzell [:nika] (ni? for response)]

I think I've figured out what's causing this - discussing potential solutions in bug 1245595.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Recent regression (from 43), tracking.

Comment 7

[TBeholder]

Still happens in FF 44.0 now.

(In reply to Elhem Enohpi from comment #2)
> And to confirm, you have cookies disabled for these sites? That agrees with
> my testing.
I just tested, you're right.
It depends on cookies - switching Cookie Controller settings to "Accept" or "Accept for a session" makes an affected tab's history work normally after the next click on a link (same host).

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Catch exceptions raised by storage.length in SessionStorage.jsm

This is a simple fix which I believe should fix the problem, and be small enough to be safe to uplift.

We still want to find the desired behavior in bug 1245595 for nightly & future releases.

(mconley: I'm not sure if you're the right person to r? on this, but I figure you'll know who to redirect to)

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ebe80820005

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8717741 [details] [diff] [review]
Catch exceptions raised by storage.length in SessionStorage.jsm

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

r=me for the SessionStorage.jsm fix only.

I have some feedback about the test though - I don't think it should land as-is. If you'd like, we can land it in a separate patch while we iterate on it.

::: browser/components/sessionstore/SessionStorage.jsm
@@ +150,5 @@
>  
>      try {
>        let storageManager = aDocShell.QueryInterface(Ci.nsIDOMStorageManager);
>        storage = storageManager.getStorage(window, aPrincipal);
> +      storage.length; // XXX: Bug 1232955 - storage.length can throw, catch that failure

I understand from reading bug 1245595 that this is urgent, but I'm curious why we even need to have access to the result of storageManager.getStorage if we can't do anything with it. Shouldn't we make it so that getStorage throws if the principal has no access to it?

(Just a curiosity, but again, due to urgency, this workaround is fine).

::: browser/components/sessionstore/test/browser_1234021.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Tests shouldn't be MPL'd - they're public domain. It's fine to leave the license off.

@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Please add:

```JavaScript
"use strict";
```

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "SessionStorage",

This test has the right idea, but it's oriented in a very non-e10s friendly way, since SessionStorage is definitely not supposed to run in the parent process, and access to contentDocuments / docShells on browsers is no bueno.

So I wouldn't import this here - what I'd suggest is that you communicate with content in order to trigger the behaviours you care about. I'll give you an example below.

@@ +9,5 @@
> +const PAGE_URL = 'http://mochi.test:8888/browser/' +
> +        'browser/components/sessionstore/test/browser_1234021_page.html';
> +const BEHAVIOR_REJECT = 2;
> +
> +let backup_pref; // Backup for restoring the preference after the test runs

sessionstore/test/head.js gives you a "pushPrefs" global that allows you to skip this book-keeping.

```JavaScript
yield pushPrefs([PREF, BEHAVIOR_REJECT]);

```

This will ensure that all processes have the pref set, and takes care of resetting the preference once the test ends.

@@ +18,5 @@
> +  yield BrowserTestUtils.withNewTab({
> +    gBrowser: gBrowser,
> +    url: PAGE_URL
> +  }, function* handler(aBrowser) {
> +    let oldCollect = SessionStorage.collect;

Never used

@@ +19,5 @@
> +    gBrowser: gBrowser,
> +    url: PAGE_URL
> +  }, function* handler(aBrowser) {
> +    let oldCollect = SessionStorage.collect;
> +    // info(Object.keys(aBrowser));

Please remove this dead code

@@ +20,5 @@
> +    url: PAGE_URL
> +  }, function* handler(aBrowser) {
> +    let oldCollect = SessionStorage.collect;
> +    // info(Object.keys(aBrowser));
> +    info(aBrowser.contentDocument.documentURIObject);

I don't think either of these info's add much.

@@ +23,5 @@
> +    // info(Object.keys(aBrowser));
> +    info(aBrowser.contentDocument.documentURIObject);
> +    info(aBrowser);
> +
> +    // Create a fake frames object to test the logic.

I assume exercising the following steps will ensure that the logic of your patch is sound:

1) Set the network.cookie.cookieBehavior pref to reject (2)
2) Have content trigger the SessionStorageInternal.collect routine
3) Have a MessageQueue flush succeed without throwing

You've already got part 1 and 2, actually, since after the frame tree is rendered for your browser_1234021_page.html, a SessionStorage collection will automatically be scheduled on a later tick of the event loop.

So what you really need to do, is just make sure that you can get a flush off of the browser, and then you're done.

Like this:

```JavaScript

yield BrowserTestUtils.withNewTab({
  gBrowser: gBrowser,
  url: PAGE_URL,
}, function* handler(browser) {
  // At this point, a SessionStorage collection should have been
  // queued. Let's make sure we can flush it out.
  yield TabStateFlusher.flush(browser);
  // mlayzell: ^-- without your patch, this should time out.
});
```

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Catch exceptions raised by storage.length in SessionStorage.jsm

I'm so sorry for the delay on this - I thought I had posted this ages ago, but then I realized I hadn't :S

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8723337 [details] [diff] [review]
Catch exceptions raised by storage.length in SessionStorage.jsm

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

If you can confirm that this test fails without your patch, then LGTM.

Comment 12

[(no longer active)]

Comment on attachment 8723337 [details] [diff] [review]
Catch exceptions raised by storage.length in SessionStorage.jsm

Approval Request Comment
[Feature/regressing bug #]: Bug 1201747
[User impact if declined]: Comment 0
[Describe test coverage new/current, TreeHerder]: Has automated tests
[Risks and why]: Should be pretty safe
[String/UUID change made/needed]: None.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

This should probably land on m-c first before uplift. We are also pretty late in the beta cycle so this may not make 45. Aurora uplift should be fine.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76b2ed3339e2

Comment 15

[Nika Layzell [:nika] (ni? for response)]

With patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=16de82999733
Without patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a7f26dbeefe

Comment 20

[xuu]

Is bug 1230769 related to this?

Comment 21

[Nika Layzell [:nika] (ni? for response)]

(In reply to xuu from comment #20)
> Is bug 1230769 related to this?

Yes, I believe that this patch should fix that problem.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76b2ed3339e2

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8723337 [details] [diff] [review]
Catch exceptions raised by storage.length in SessionStorage.jsm

This is too late in the 45 cycle as we are now doing RC. We already shipped 44 with it.
But taking it in 46.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee780025bbf4