Closed Bug 1026614 Opened 10 years ago Closed 8 years ago

SDK ui/toolbar not working in permanent private browsing

Categories

(Add-on SDK Graveyard :: General, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Kinneer_Erin, Assigned: zer0)

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

Using the toolbar ui component example in the add-on sdk, I gave the toolbar extension permissions to be used in private browsing. In "permanent private browsing", the extension disappears. I need it to show in both "permanent private browsing" and window private browsing. 


Actual results:

The extension disappeared. 


Expected results:

The toolbar extension should have been in the browser.
Component: Untriaged → General
OS: Windows 7 → All
Product: Firefox → Add-on SDK
Hardware: x86_64 → All
Version: 30 Branch → unspecified
Flags: needinfo?(zer0)
Can confirm this is a bug, and also give you the places of where things go wrong.

https://github.com/mozilla/addon-sdk/blob/f6bc93143d72c8a498099b39d8ddaf98a2071f52/lib/sdk/input/browser.js#L20
https://github.com/mozilla/addon-sdk/blob/f6bc93143d72c8a498099b39d8ddaf98a2071f52/lib/sdk/window/utils.js#L296

In input/browser.js, |windows| is called with |includePrivates: true|, while the actual option name is |includePrivate|. Hence private windows not enumerated to begin with, so no toolbars are ever created for private windows.

STR:
* Build a new add-on using the attached main.js and a package.json containing |"permissions": {"private-browsing": true}| (See: https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/private-browsing)
* cfx run
* Open the browsr Preferences/Privacy and switch to Firefox will: Never remember history (aka. permanent private browsing)
* Browser prompts to restart: Confirm
-> After the browser restarts, the toolbar is missing in the initial window(s).
-> Opening new windows will display the toolbar in these windows

Expected:
-> After the browser restarts, the toolbar is present and functional.


But this is not all:
After fixing the above, the toolbar will *always* show up, as the toolbar code does not consider the private-windows permission at all, e.g. by using |ignoreWindow|.
ActionButtons do evaluate the private-browsing permission, hence the Toolbar will be empty in private windows.

STR:
* Fix the first bug, by correcting the option name in input/browser.js :p
* In package.json, remove |"permissions": {"private-browsing": true}| again
* cfx run (make sure the patched SDK is used)
* Open a new private window
-> An empty toolbar is displayed in the private window

Expected:
-> No toolbar should be displayed in the private window
Status: UNCONFIRMED → NEW
Ever confirmed: true
Here is a quick and dirty patch.

Please note however, that I'm not interested in taking this bug, going threw reviews, writing test, etc. The patch is purely for the information of whoever will fix this bug eventually.
Comment on attachment 8445722 [details] [diff] [review]
Quick and dirty patch against the current master

Matteo, care to review this?
Attachment #8445722 - Flags: review?(zer0)
This bug appears to also cause this code to open a new window instead of using the currently open "permanent private browsing" window.

tabs.open({
    url: url_to_open,
    isPrivate: true,
    inNewWindow: false
});
Luca, could you have a look? It's a quick review, we should probably check more edge cases, but at least it's better than now.
Flags: needinfo?(zer0)
Attachment #8731869 - Flags: review?(lgreco) → review+
Comment on attachment 8731869 [details]
MozReview Request: Bug 1026614 - SDK ui/toolbar not working in permanent private browsing; r=rpl

https://reviewboard.mozilla.org/r/40889/#review37571
Attachment #8445722 - Attachment is obsolete: true
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #6)
> Luca, could you have a look? It's a quick review, we should probably check
> more edge cases, but at least it's better than now.

The change and the new test cases looks good.

I'd like to have another test case that actually tests that the toolbar is shown when the addon has the private-browsing permission (which would ensure that the issue described in the summary is fixed), but it seems that the test addon that should test this scenarios as been disabled:

https://dxr.mozilla.org/mozilla-central/rev/3e04659fdf6aef792f7cf9840189c6c38d08d1e8/addon-sdk/source/test/addons/jetpack-addon.ini#34

I tried to fix its most evident issue:

   const BUILTIN_SIDEBAR_MENUITEMS = exports.BUILTIN_SIDEBAR_MENUITEMS = [
     'menu_socialSidebar',
     'menu_historySidebar',
     'menu_bookmarksSidebar',
+    'menu_tabsSidebar',
   ];


and to add the new test case for the ui toolbar, and it seems possible to fix and renable it. 

are there other reasons why the private-browsing-supported test addon is currently skipped on try?
Flags: needinfo?(zer0)
(In reply to Luca Greco [:rpl] from comment #8)

> are there other reasons why the private-browsing-supported test addon is
> currently skipped on try?

I don't know, I don't have much experience on add-on sdk tests on try since we moved from github. Probably Mossop knows better, since it seems the one who landed those changes.
In the worst scenario, if no one knows, we can always try to re-enable and see what's happening. But if it's becoming too complicated, I would suggest to file a new bug for that and land in the meantime this one.

Mossop, do you recall anything about it?
Flags: needinfo?(zer0) → needinfo?(dtownsend)
If I knew then I sure don't remember now
Flags: needinfo?(dtownsend)
(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #9)
> But if it's becoming too complicated, I would suggest
> to file a new bug for that and land in the meantime this one.

sure, the test that you added ensures that the current behavior is preserved when it should be (when the addon doesn't have a private-browsing permission), and I'm ok with handling the opposite scenario (when the addon has the private-browsing permission and the toolbar should be attached) in a separate bugzilla issue.
Assignee: nobody → zer0
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/577b8fb1479a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.