arifwn / TinySuspender

Unload idle tabs to reduce memory and cpu usage.
55 stars 11 forks source link

Required permission (read all your data) #2

Closed extraneu closed 1 year ago

extraneu commented 7 years ago

Hi,

the extension version in the Web store asks for permission "Read and modify all your data on all websites you visit", do you know why is that? I've looked at the manifest from the master branch and I can't see any permission in it that would trigger this message.

Best regards, Flávio

arifwn commented 7 years ago

I think it's the tabs and activeTab permissions that trigger the message. Those are required for accessing urls data.

extraneu commented 7 years ago

AFAICS from Google documentation it shouldn't be the case: https://developer.chrome.com/extensions/activeTab https://developer.chrome.com/extensions/permission_warnings

But yeah, Google documentation WRT to Chrome is often outdated :-/ Thanks anyway.

filbo commented 6 years ago

'Read and change all your data on the websites you visit' is due to "content_scripts": [ { "matches": [ "*://*/*" ] } ] (this is clear from the current version of https://developer.chrome.com/extensions/permission_warnings); and probably cannot be avoided. Any extension which needs a content script running in all tabs gets that warning; and the warning is true -- if the author of the extension put malicious code in the content script, he could do anything to any tab.

Can this extension work with only activeTab permission? Probably not. Can it work with activeTab + tabs, without the content script injected into all pages? Looks like the content script's purpose is to save form inputs and tab scroll position. So I suppose there could be a user-settable option to not preserve those details, and not require a content script; but would anyone actually want a suspender which loses input fields?

If the native-discard back end works, perhaps the content script isn't needed with that. It handles input fields & scroll positions for you, doesn't it?

arifwn commented 6 years ago

Yes, if you use chrome tab discard api, content script is not needed anymore. But tabs permissions is still needed for suspending background tabs.

I actually use this extension with native tab discard enabled, and I think it works better than using the default page-based suspension. There are some drawbacks though: it can't suspend active tab, and suspended tab will be restored immediately when you switch to them. Because of these drawbacks, I don't think I can remove the page-based tab suspension for now (and thus can't remove the content script) as some people want control over this behavior.

Unfortunately, content script is always loaded when the regex matches the tab's url. But maybe I can update the content script to not load any code if native tab discard option is enabled by moving the code to a separate module. This could potentially reduce extension's memory footprint as well when native tab discard is enabled.

filbo commented 6 years ago

I'm currently using it in Opera. Opera apparently doesn't support chrome.tab.discard() despite being based on fresh builds of Chromium. See discussion under https://blogs.opera.com/desktop/2018/10/opera-57-0-3098-1-developer-update/#comment-4133203273

So I'd be in favor of not removing the old-style tab management option, given that the new doesn't work at all in some instances.

(I wonder if you could, at install time, fool around with creating a new tab, suspending it with discard, checking the return from the various APIs to confirm that it really is 'discarded'. Then initialize the discard-vs-suspend setting based on the results.)

I came here to report a new issue, but will first briefly mention it here in case you have a quick answer. It is that the setting 'Automatically restore tab when brought to foreground' doesn't seem to work.

Tabs are suspending correctly; persisting correctly across browser shutdown / restart; and resuming correctly. But even with that setting turned on since browser restart, the only way to wake a tab is to switch to it, then click in the top blue area.

(Which reminds me of the smaller issue: the words 'This tab is currently suspended. Click anywhere to restore.' are incorrect: clicking those words themselves does not wake the tab. Only clicking in the blue area does. This gave me a bit of a panic at first when I saw that tabs had been suspended and -- what the heck! they won't wake up! -- oh, ok, I have to click in the blue area not 'anywhere'...) (In fact, now that I focus on that: I see the mouse pointer changing between 'hand' and 'pointer' as I move around the blue area. It isn't even the entire blue area which will wake the tab; only the text of the title & URL.) Please change it to agree with the words -- the entire blue & white areas should be active.

arifwn commented 6 years ago

Your issues seem to be specific to Opera. I can't reproduce it in Chrome. Both 'Automatically restore tab when brought to foreground' and 'Click anywhere to restore.' do work correctly on chrome.

Actually, I never tested this extension in other browsers before. I'm surprised it's actually work at all on Opera! Maybe Opera handles tabs.onActivated event differently (thus suspended tab not restoring automatically when brought to foreground). Also, Opera might also render viewport differently. The page used to display suspended message has ~150px height. In Chrome, clicking below that point (below blue area) still counts as clicking the document, while Opera probably ignore click event below that point.

filbo commented 6 years ago

Thanks -- knowing that it works as expected in Chrome gives me a basis to debug it. Will get back to it in a couple of days.

As current Opera is a mutated Chromium, it isn't that surprising it works. They slipstream new Chromium releases into their build all the time, so we know it isn't too radically changed. But yeah, they do have a significant change in just the 'right' place (tab API) to potentially screw this up...