899276 - [Session Restore] Don't collect/save private tabs

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Since each docShell has an attribute |usePrivateBrowsing|, we should make sure that we do not collect/save private tabs.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs

Here's a simple implementation.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 783203 [details] [diff] [review]
Don't collect private tabs

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1337,5 @@
>      }
>  
> +    // Private tabs should not be saved
> +    let loadContext = aTab.linkedBrowser.docShell.QueryInterface(Ci.nsILoadContext);
> +    if (loadContext.usePrivateBrowsing) {

_collectBaseTabData() indicates that aTab.linkedBrowser might be undefined. The same is probably true for the docShell property. You could put this in a simple helper function at the top, like isPrivateTab().

@@ +2589,5 @@
>     *        Bool collect pinned tabs only
>     * @returns object
>     */
> +  _getCurrentState: function ssi_getCurrentState(aOptions = {}) {
> +    let includeAllWindows = !!aOptions.includeAllWindows;

includeAllWindows is quite a misleading name. In fact it rather is about *updating* data for all windows. They all are included anyway.

@@ +2662,2 @@
>        // perform a deep copy so that existing session variables are not changed.
>        total = JSON.parse(this._toJSONString(total));

Sorry for the rot but this has been removed \o/.

@@ +2740,5 @@
>      // update the internal state data for this window
>      for (let tab of tabs) {
> +      // Generally, we want to skip private tabs
> +      if (!includePrivateTabs) {
> +        let loadContext = tab.linkedBrowser.docShell.QueryInterface(Ci.nsILoadContext);

I think we should just always collect data for private tabs but remove them in .saveState() just like we do for private windows. It will certainly help if we'd add a .private property to tabData to make the filter step a little faster. Also because that .private flag will be sent with the data collected in the content process (in the future).

Comment 3

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #2)
> 
> I think we should just always collect data for private tabs but remove them
> in .saveState() just like we do for private windows. It will certainly help
> if we'd add a .private property to tabData to make the filter step a little
> faster. Also because that .private flag will be sent with the data collected
> in the content process (in the future).

If we do this, getBrowserState(), etc. will return the private tabs. Is this what you want?

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v2

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> If we do this, getBrowserState(), etc. will return the private tabs. Is this
> what you want?

Yes, just like it includes private windows.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 811369 [details] [diff] [review]
Don't collect private tabs, v2

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

Patch needs rebasing.

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +234,5 @@
>        }
>      }
>  
> +    // Removed private tabs from the list of closed tabs.
> +    for (let i = state.windows.length - 1; i >= 0; i--) {

We don't need to do that because we never put them into _closedTabs in the first place, right?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +4315,5 @@
>    },
>  
> +  _isPrivateTab: function(browser) {
> +    return (browser  && browser.docShell &&
> +        browser.docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing);

Nit: two spaces and we don't really need the parenthesis.

@@ +4369,5 @@
>      }
>  
> +    if (TabState._isPrivateTab(browser)) {
> +      tabData.isPrivate = true;
> +    } // Don't set anything if the tab is not private, to save space

Nit: the comment at the end looks weird

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v3

Applied feedback.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 820233 [details] [diff] [review]
Don't collect private tabs, v3

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

How up-to-date is your repo? browser/components/sessionstore/test/Makefile.in doesn't exist anymore here.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v4

Rebased

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

What does this patch do? I thought we already handled private windows specially. Do we ever have private tabs that belong to non-private windows?

I'm worried because this isn't going to work in e10s since it touches the docshell. We could find a way to do it, but it will be more work.

Comment 11

[(no longer active)]

(In reply to Tim Taubert [:ttaubert] from comment #5)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> > If we do this, getBrowserState(), etc. will return the private tabs. Is this
> > what you want?
> 
> Yes, just like it includes private windows.

That seems really bad.  Why is that desired?

Comment 12

[David Teller [:Yoric] - still alive but not very active]

(In reply to Bill McCloskey (:billm) from comment #10)
> What does this patch do? I thought we already handled private windows
> specially. Do we ever have private tabs that belong to non-private windows?

We have private tabs, they are just not exposed to the UI. The reason we want this bug fixed is because there is a nice (and popular) add-on that adds the missing UI to Firefox, and this add-on works around the lack of support for private tabs in Session Restore by [ab]using a not-quite-documented notification that was included in Session Restore. We need to get rid of that notification for bug 886447, which should provide an additional performance boost.

> I'm worried because this isn't going to work in e10s since it touches the
> docshell. We could find a way to do it, but it will be more work.

Sounds like I should rewrite my patch to get the information child-side, then.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Bill McCloskey (:billm) from comment #10)
> I'm worried because this isn't going to work in e10s since it touches the
> docshell. We could find a way to do it, but it will be more work.

Good point. Thanks for catching this! It's all too easy to forget about e10s :/

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #11)
> (In reply to Tim Taubert [:ttaubert] from comment #5)
> > (In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> > > If we do this, getBrowserState(), etc. will return the private tabs. Is this
> > > what you want?
> > 
> > Yes, just like it includes private windows.
> 
> That seems really bad.  Why is that desired?

I wouldn't say it's desired but that's just the way it's been ever since we had private windows. I think it is the correct thing to do for getBrowserState() to return the full state including private windows and tabs. Just like it returned the private state back when we had the PB *mode* (iirc).

The method just returns the current state and it's the caller's responsibility what do to with that data now. We only guarantee to never save sensitive data to disk and that we won't be able to restore it.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 820244 [details] [diff] [review]
Don't collect private tabs, v4

We of course need something e10s compatible. Sorry that I didn't think of this earlier.

Comment 15

[(no longer active)]

(In reply to comment #13)
> (In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #11)
> > (In reply to Tim Taubert [:ttaubert] from comment #5)
> > > (In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> > > > If we do this, getBrowserState(), etc. will return the private tabs. Is this
> > > > what you want?
> > > 
> > > Yes, just like it includes private windows.
> > 
> > That seems really bad.  Why is that desired?
> 
> I wouldn't say it's desired but that's just the way it's been ever since we had
> private windows. I think it is the correct thing to do for getBrowserState() to
> return the full state including private windows and tabs. Just like it returned
> the private state back when we had the PB *mode* (iirc).

Hmm, I think that is my oversight.  :(  It's not necessarily what I wanted to happen here.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to :Ehsan Akhgari (needinfo? me!) [Away 10/29-11/6] from comment #15)
> > I wouldn't say it's desired but that's just the way it's been ever since we had
> > private windows. I think it is the correct thing to do for getBrowserState() to
> > return the full state including private windows and tabs. Just like it returned
> > the private state back when we had the PB *mode* (iirc).
> 
> Hmm, I think that is my oversight.  :(  It's not necessarily what I wanted
> to happen here.

We can of course re-think that decision. I still think it's not a big deal to include private state in getCurrentState (and IMO that's "correct" in terms of returning the current state) but we might as well always exclude it if that's the better thing to do from a privacy point of view.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

In terms of the whole issue here, it seems that setting .usePrivateBrowsing is explicitly discouraged since bug 800193. bug 800193 comment #2 states that this might not at all be properly supported. Should we still support filtering out private tabs?

If we're going to do this we should implement it based off of bug 919835 and collect the .private flag in the frame script to not break e10s.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Well, we are going to kill the Private Tabs add-on if we don't fix this bug. This is a maintained, very active, add-on, whose author is quite responsive. It would be a shame to do that.

So, I would vote for collecting private flag in the frame script.

Comment 19

[(no longer active)]

(In reply to comment #17)
> In terms of the whole issue here, it seems that setting .usePrivateBrowsing is
> explicitly discouraged since bug 800193. bug 800193 comment #2 states that this
> might not at all be properly supported. Should we still support filtering out
> private tabs?

Yes, we should try to handle those cases properly when we can.

Comment 20

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

This will change to that we cannot restore tabs with "undo close tab" in private mode window, won't it?

If it's so, I think this would cut off the useful feature. Chromium has the same "undo close tab" feature excluded in their private browsing windows. But it's not useful. Because we cannot restore the tab even if we close tabs by our mistake.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #20)
> This will change to that we cannot restore tabs with "undo close tab" in
> private mode window, won't it?

No, it won't. This is about private tabs in non-private windows that currently are saved to disk because we just don't expect them. This is only possible using add-ons as vanilla Firefox doesn't expose any UI to create private tabs (in a non-private window).

Comment 22

[David Teller [:Yoric] - still alive but not very active]

I believe that the right way to move this to the child process is to wait for bug 930967.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v5

Rewrote the patch to make it e10s-friendly.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8338474 [details] [diff] [review]
Don't collect private tabs, v5

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

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +322,5 @@
> +  },
> +
> +  // Ci.nsIPrivacyTransitionObserver
> +  privateModeChanged: function(enabled) {
> +    MessageQueue.push("isPrivate", () => enabled?true:undefined);

I would like (enabled || null) better as that's what the other collection routines use to signal 'no value'. Also, TabStateCache.updatePersistent() checks for === null.

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +206,5 @@
> +      }
> +      // The window is not private, but its tabs still might
> +      for (let j = win.tabs.length - 1; j >= 0 ; --j) {
> +        let tab = win.tabs[j];
> +        if (tab.isPrivate) {

if (tab.isPrivate || false) to avoid a warning in strict mode when the property doesn't exist.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1306,5 @@
> +    // Don't save private tabs
> +    if (browser && browser.docShell &&
> +        browser.docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing) {
> +      return;
> +    }

We first need to flush (which is done below), then you can use |tabState.isPrivate| to see whether the tab has a private docShell. The code above doesn't work in e10s.

::: browser/components/sessionstore/test/browser.ini
@@ +57,5 @@
>  [browser_global_store.js]
>  [browser_input.js]
>  [browser_pageshow.js]
>  [browser_pageStyle.js]
> +[browser_privatetabs.js]

I think you forgot add the test file.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v6

Applied feedback, except the flushing bit I don't understand.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v7

Ok, got it.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8338508 [details] [diff] [review]
Don't collect private tabs, v7

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

Thanks, looking good!

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +313,5 @@
> + * By definition, tabs start in non-private mode.
> + *
> + * Causes a SessionStore:update message to be sent that contains
> + *  |true| if the tab is now private
> + *  |false| if the tab is now public.

Maybe explain that we don't actually send the 'false' value to not store an .isPrivate=false property on every tab.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +1308,5 @@
>      // Get the latest data for this tab (generally, from the cache)
>      let tabState = TabState.collectSync(aTab);
>  
> +    // Don't save private tabs
> +    if (tabState.isPrivate) {

if (tabState.isPrivate || false) :)

::: browser/components/sessionstore/test/browser_privatetabs.js
@@ +17,5 @@
> +    info("Setting up private tab");
> +    tab2 = gBrowser.addTab();
> +    yield promiseBrowserLoaded(tab2.linkedBrowser);
> +    let loadContext = tab2.linkedBrowser.docShell.QueryInterface(Ci.nsILoadContext);
> +    loadContext.usePrivateBrowsing = true;

Can you use the /test/content.js frame script we now have for testing? This would otherwise break the test in e10s.

@@ +22,5 @@
> +    tab2.linkedBrowser.loadURI("about:config");
> +    yield promiseBrowserLoaded(tab2.linkedBrowser);
> +
> +    info("Checking out state");
> +    yield forceSaveState();

We need a sync flush before this line here to ensure all the data has arrived at the chrome process before saving to disk.

@@ +30,5 @@
> +    ok(state.indexOf("about:robots") != -1, "State contains public tab");
> +    ok(state.indexOf("about:config") == -1, "State does not contain private tab");
> +
> +    // Flush to make sure chrome received all data.
> +    SyncHandlers.get(tab2.linkedBrowser).flush();

gBrowser.removeTab() flushes synchronously, we don't need that line.

@@ +46,5 @@
> +    let canUndo = false;
> +    try {
> +      tab2 = ss.undoCloseTab(window, 0);
> +      canUndo = true;
> +    } catch (ex if ex = Cr.NS_ERROR_ILLEGAL_VALUE) {

Might be easier to check that ss.getClosedTabCount() == 0.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8338508 [details] [diff] [review]
Don't collect private tabs, v7

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

::: browser/components/sessionstore/test/browser_privatetabs.js
@@ +46,5 @@
> +    let canUndo = false;
> +    try {
> +      tab2 = ss.undoCloseTab(window, 0);
> +      canUndo = true;
> +    } catch (ex if ex = Cr.NS_ERROR_ILLEGAL_VALUE) {

Also, when checking the number of tabs available to undoClose, you should do |while (ss.getClosedTabCount()) ss.forgetClosedWindow(0)| at the beginning of the test. This would probably break when running the whole test suite otherwise.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v8

Applied feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=a499f2b97782

Comment 30

[Tim Taubert [:ttaubert] (inactive)]

Thanks, the changes look good. Two nits to address before you push, please:

+++ b/browser/components/sessionstore/test/browser_privatetabs.js
+    info("Setting up private tab");
+    tab2 = gBrowser.addTab();
+    yield promiseBrowserLoaded(tab2.linkedBrowser);
+    let loadContext = tab2.linkedBrowser.docShell.QueryInterface(Ci.nsILoadContext);

That unused loadContext variable would still break us in e10s but I assume you just forgot to remove that line.

+    ok(false, "Unexpected rrror: " + ex);
+    info(ex.stack);

*error. I would be ok with errror as well ;)

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v9

Applied feedback.

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/c284670371cc

Comment 33

[Carsten Book [:Tomcat]]

(In reply to Tim Taubert [:ttaubert] from comment #32)
> https://hg.mozilla.org/integration/fx-team/rev/c284670371cc

sorry, had to back this out for causing bc failures in Windows 7 like https://tbpl.mozilla.org/php/getParsedLog.php?id=31262090&tree=Fx-Team

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

I believe that this has the same cause as bug 919060. A background collection is saving state to disk that is stale - from the test's point of view.

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

You could maybe call SessionSaver.run()? That works synchronously and should be perfect for a test as long as we still have the async background collection.

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v10

Tweaked the test suite into compliance: https://tbpl.mozilla.org/?tree=Try&rev=40c91afa78e9

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

The test fails locally for me. There is at least one failure on your try run as well.

Comment 38

[David Teller [:Yoric] - still alive but not very active]

I'll take a look, thanks.

Comment 39

[David Teller [:Yoric] - still alive but not very active]

Looking at the logs, it seems that SessionSaver.run() produces a sessionstore.js that is completely wrong – presumably tuned to a previous state of the session store. That's a little worrying.

Comment 40

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v11

Ah, got it. Race condition in the test.
Tim, I extended SessionSaver.run to return a promise, to ensure that we can synchronize with the full run, including the write. Is that ok with you?

Comment 41

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8348260 [details] [diff] [review]
Don't collect private tabs, v11

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

::: browser/components/sessionstore/test/browser_privatetabs.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +let Imports = {};
> +Cu.import("resource://gre/modules/Task.jsm", Imports);
> +Cu.import("resource://gre/modules/Promise.jsm", Imports);

You don't seem to use Task.jsm or Promise.jsm here explicitly.

@@ +26,5 @@
> +
> +    info("Setting up private tab");
> +    tab2 = gBrowser.addTab();
> +    yield promiseBrowserLoaded(tab2.linkedBrowser);
> +    setUsePrivateBrowsing(tab2.linkedBrowser, true);

This should be yield setUsePrivateBrowsing()

@@ +35,5 @@
> +    SyncHandlers.get(tab2.linkedBrowser).flush();
> +
> +    info("Checking out state");
> +    yield SessionSaver.run();
> +    let state = new TextDecoder().decode((yield OS.File.read(OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.js"))));

Nit: can you please split this into multiple lines to make it more readable?

::: browser/components/sessionstore/test/content.js
@@ +54,5 @@
> +addMessageListener("ss-test:setUsePrivateBrowsing", function (msg) {
> +  let loadContext =
> +    docShell.QueryInterface(Ci.nsILoadContext);
> +  loadContext.usePrivateBrowsing = msg.data;
> +  sendSyncMessage("ss-test:setAuthorStyleDisabled");

Please use sendAsyncMessage() and send the right message name.

Comment 42

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #41)
> ::: browser/components/sessionstore/test/content.js
> @@ +54,5 @@
> > +addMessageListener("ss-test:setUsePrivateBrowsing", function (msg) {
> > +  let loadContext =
> > +    docShell.QueryInterface(Ci.nsILoadContext);
> > +  loadContext.usePrivateBrowsing = msg.data;
> > +  sendSyncMessage("ss-test:setAuthorStyleDisabled");
> 
> Please use sendAsyncMessage() and send the right message name.

Good catch. It's a little worrying that this worked, though.

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #42)
> Good catch. It's a little worrying that this worked, though.

The load started before you set the docShell to private but by the time the load finished the frame script already received the message. So not too worrisome :)

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Attachment: Don't collect private tabs, v12

Applied feedback.
Try: https://tbpl.mozilla.org/?tree=Try&rev=8da759304ba3

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7289c4384fc7

Comment 46

[Tim Taubert [:ttaubert] (inactive)]

Pushed a follow-up to fix mochitest-bc perma-orange:

https://hg.mozilla.org/integration/fx-team/rev/638d0be07a5f

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7289c4384fc7
https://hg.mozilla.org/mozilla-central/rev/638d0be07a5f

Comment 48

[Infocatcher]

Hmmm, doesn't work for me sometimes.
Testcase for Scratchpad (devtools.chrome.enabled = true + Environment – Browser):


// Works fine, private tab isn't saved:
window.addEventListener("TabOpen", function makeTabPrivate(e) {
	window.removeEventListener("TabOpen", makeTabPrivate, false);
	var tab = e.target;
	var browser = tab.linkedBrowser;
	var privacyContext = PrivateBrowsingUtils.privacyContextFromWindow(browser.contentWindow);
	privacyContext.usePrivateBrowsing = true;
	// Wait some time and try restart
	browser.addEventListener("load", function waitForLoading(e) {
		browser.removeEventListener("load", waitForLoading, true);
		setTimeout(function() {
			Application.restart();
		}, 5e3);
	}, true);
}, false);
gBrowser.addTab("https://addons.mozilla.org/");


// Doesn't work, private tab saved in the session:
window.addEventListener("TabOpen", function makeTabPrivate(e) {
	window.removeEventListener("TabOpen", makeTabPrivate, true /* !!! */);
	var tab = e.target;
	var browser = tab.linkedBrowser;
	var privacyContext = PrivateBrowsingUtils.privacyContextFromWindow(browser.contentWindow);
	privacyContext.usePrivateBrowsing = true;
	// Wait some time and try restart
	browser.addEventListener("load", function waitForLoading(e) {
		browser.removeEventListener("load", waitForLoading, true);
		setTimeout(function() {
			Application.restart();
		}, 5e3);
	}, true);
}, true /* !!! */);
gBrowser.addTab("https://addons.mozilla.org/");

Comment 49

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Infocatcher from comment #48)
> Hmmm, doesn't work for me sometimes.

Can you please file a new bug for this issue? I assume that the docShell is made private before the frame script is able to register its listener. An easy fix would be to check whether the docShell is already private in PrivacyListener.init() and call MessageQueue.push() if so.

Comment 50

[Infocatcher]

(In reply to Tim Taubert [:ttaubert] from comment #49)
> Can you please file a new bug for this issue?
Done: https://bugzilla.mozilla.org/show_bug.cgi?id=956826