Expose SessionStoreInternal to a list of allowed callers




6 years ago
5 years ago


(Reporter: ttaubert, Assigned: ttaubert)



Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

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.
Created attachment 788634 [details] [diff] [review]
Expose SessionStoreInternal to a list of allowed callers
Attachment #788634 - Flags: review?(dteller)
Created attachment 788635 [details] [diff] [review]
Expose SessionStoreInternal to a list of allowed callers, v2

Small fix.
Attachment #788634 - Attachment is obsolete: true
Attachment #788634 - Flags: review?(dteller)
Attachment #788635 - Flags: review?(dteller)
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.
... 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.
I don't really like the mechanism. I'll try and think this over.
Yeah, me neither (anymore). I'm a little out of ideas, though.
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?
Attachment #788635 - Flags: review?(dteller) → review+
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.


6 years ago
No longer blocks: 894595
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.
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.