903828 - Expose SessionStoreInternal to a list of allowed callers

Status: RESOLVED WONTFIX

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

We already expose SessionStoreInternal via SessionStore._internal but only when debug mode is enabled.

SessionStore.checkPrivacyLevel() is a method that has been exposed a little while ago to allow SessionStorage.jsm to use it. It still is an internal method and API though that third-party code shouldn't use and rely on.

For the implementation of bug 894595 I would need to expose _getCurrentState() and would really actually limit allowed callers to be the test suites and our internal modules. That would help a lot with modularizing our sessionstore code, too.

I think we can use stack inspection to retrieve the current caller if we make sure to cache SessionStoreInternal in the calling modules. The overhead here should really be negligible.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Expose SessionStoreInternal to a list of allowed callers

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Expose SessionStoreInternal to a list of allowed callers, v2

Small fix.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Hmm. I'm not so sure about this anymore. I still think it would be good to restrict callers to an API that we want to expose but I think that probably also affects the modules we created for SessionStore.

Ideally, we'd only want to expose SessionStore.internal and its module like SessionFile, the SessionWorker, SessionCookies, DocumentUtils, etc. to our internal code.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

... because otherwise we'll just end up like with PageThumbs.jsm. The module wasn't even finished yet we already had to maintain functions for backwards compatibility and mark them as deprecated.

I'm not sure we can really do anything about that, though.

Comment 5

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

I don't really like the mechanism. I'll try and think this over.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Yeah, me neither (anymore). I'm a little out of ideas, though.

Comment 7

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

Comment on attachment 788635 [details] [diff] [review]
Expose SessionStoreInternal to a list of allowed callers, v2

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

I'm not a big fan of the approach, but attempting to make this cleaner (e.g. using capabilities) would probably require a large refactoring, so let's go along with it.

Perhaps we should file a (followup) bug to get a general mechanism of authorizations as either wrappers or a module.

::: browser/components/nsBrowserGlue.js
@@ +604,5 @@
>        temp.WinTaskbarJumpList.startup();
>      }
>  #endif
>  
> +    SessionStoreInternal.init(aWindow);

So, the objective in this file is to hide |init|?
Is that better than making it idempotent?

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 788635 [details] [diff] [review]
Expose SessionStoreInternal to a list of allowed callers, v2

>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js

>+XPCOMUtils.defineLazyGetter(this, "SessionStoreInternal", () => {
>+  Cu.import("resource:///modules/sessionstore/SessionStore.jsm");
>+  return SessionStore.internal;
>+});

You're leaking 'SessionStore' into the global scope here.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Marking as WONTFIX. Should we ever get around to implement something like this, which I would really appreciate, we probably would want a broader solution.