data manager: domains invisible for Permissions, Preferences, Passwords,

RESOLVED FIXED in seamonkey2.45

Status

defect
--
major
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: chokito, Assigned: frg)

Tracking

({useless-UI})

SeaMonkey 2.39 Branch
seamonkey2.45
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.39 wontfix, seamonkey2.40 wontfix, seamonkey2.41 wontfix, seamonkey2.42 wontfix, seamonkey2.43 wontfix, seamonkey2.44 wontfix, seamonkey2.46 fixed)

Details

(Whiteboard: [workaround see comments #30 and #92])

User Story

Only for symptoms based of lacking adaption of SeaMonkey to Gecko 42

Attachments

(8 attachments, 13 obsolete attachments)

81.93 KB, image/png
Details
9.51 KB, patch
kairo
: review+
Details | Diff | Splinter Review
28.42 KB, image/png
Details
3.91 KB, image/png
kairo
: feedback+
Details
48.61 KB, application/zip
Details
142.50 KB, image/jpeg
Details
7.86 KB, patch
Details | Diff | Splinter Review
100.27 KB, patch
iann_bugzilla
: review+
Details | Diff | Splinter Review
Reporter

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0 SeaMonkey/2.39a1
Build ID: 20150728011950

Steps to reproduce:

Select Tools > Data Manager or Tools > Password Manager > Manage Stored Passwords.


Actual results:

Showing none of the stored passwords.


Expected results:

Showing the stored data.
(The data are visible under chrome://passwordmgr/content/passwordManager.xul).

Comment 1

4 years ago
@reporter:
If we observe the same problem the core of the problem is that wrongly no domains with saved passwords are listed in the domain pane?
Flags: needinfo?(chokito)

Comment 2

4 years ago
OS due to current knowledge.

a) Observations as per att. 2015-07-28 16:42 CEST, Rainer Bielefeld 
   Already reproducible with:
   User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101
   Firefox/42.0 SeaMonkey/2.39a1 Build identifier: 20150721221150
b) Observations still ok:
   User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101
   Firefox/42.0 SeaMonkey/2.39a1
   Build identifier: 20150629193856
c) No DUPs found in <https://bugzilla.mozilla.org/buglist.cgi?cmdtype=dorem&remaction=run&namedcmd=DUPs1188348&sharer_id=41036&list_id=12427189>
OS: Unspecified → Windows 7
Reporter

Comment 3

4 years ago
(In reply to Rainer Bielefeld from comment #1)
> Created attachment 8639864 [details]
> No domains with passwords?
> 
> @reporter:
> If we observe the same problem the core of the problem is that wrongly no
> domains with saved passwords are listed in the domain pane?

Yes its the same problem. No domains listed.
Flags: needinfo?(chokito)

Comment 4

4 years ago
d) not only Passwords affected
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Stored passwords invisible → data manager: domains invisible for Permissions, Preferences, Passwords,

Comment 5

4 years ago
Could someone check if the same happens with Firefox Nightly and Data Manager add-on?

Comment 6

4 years ago
With FF 42.0a1 (2015-08-02 Tahoe Data Manager 1.6.1-signed shows the same symptoms to me as shown in screenshot 2015-07-28 16:42 CEST, Rainer Bielefeld

Comment 7

4 years ago
(In reply to Rainer Bielefeld from comment #6)
Robert, can you help from add-on's point of view?
Reporter

Comment 8

4 years ago
The same with the newest SeaMonkey 2.39a1 version.
User agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:42.0) Gecko/20100101 Firefox/42.0 SeaMonkey/2.39a1
Build identifier: 20150805174223

Updated

4 years ago
Duplicate of this bug: 1194681

Comment 10

4 years ago
(In reply to Rainer Bielefeld from comment #7)
> (In reply to Rainer Bielefeld from comment #6)
> Robert, can you help from add-on's point of view?

No, I just don't have time to look into anything, see also https://home.kairo.at/blog/2015-08/ending_development_and_support_for_my_ad

Updated

4 years ago
See Also: → 1209622

Updated

4 years ago
See Also: → 1209626

Updated

4 years ago
See Also: 1209622
Duplicate of this bug: 1209622

Updated

4 years ago
See Also: 1209626
Duplicate of this bug: 1209626
Assignee

Comment 14

4 years ago
I reopened bug 1209626.

This one is already broken in 2.38. If you don't think so close it. I will check it out again after 1188348 has been fixed. Might be somehow related to the Firefox bug 1170200 referenced in bug 1208700.
Assignee

Comment 15

4 years ago
I think bug 1173523 and bug 1165263 are the root cause of this.

Comment 16

4 years ago
I did not test 2.40
This bug happens also on Linux, see duplicate bug 1208700
Severity: normal → major
OS: Windows 7 → All
Hardware: Unspecified → All
Version: SeaMonkey 2.39 Branch → Trunk
Assignee

Comment 19

4 years ago
Digged a little in the Firefox code and found a simple solution for extracting the hostname. Not a complete patch. Removal and other actions still broken.
Assignee

Comment 20

4 years ago
Posted patch 1188348-permission-V2.patch (obsolete) — Splinter Review
This patch seems to work. Removal is possible. I still see duplicate cookie sites in the cookieviewer.js sites list but removing them works so they seem to be really different. Maybe someone else can look into this.
Attachment #8672271 - Attachment is obsolete: true

Comment 21

4 years ago
Comment on attachment 8672371 [details] [diff] [review]
1188348-permission-V2.patch

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

You should ask someone for review - either me as I guess I still own Data Manager, or someone else who can do SeaMonkey UI reviews (Neil or similar).

As you did not ask review, I'll give you f+ based on code inspection, pending addressing my other comments.

::: suite/common/dataman/dataman.js
@@ +284,5 @@
>        gDomains.ignoreUpdate = true;
>        let enumerator = Services.perms.enumerator;
>        while (enumerator.hasMoreElements()) {
>          let nextPermission = enumerator.getNext().QueryInterface(Components.interfaces.nsIPermission);
> +        gDomains.addDomainOrFlag(nextPermission.principal.URI.host.replace(/^\./, ""), "hasPermissions");

Can those still start with a "."? If not, then we probably can simplify all those calls.

::: suite/common/permissions/cookieViewer.js
@@ +400,5 @@
>          default:
>            continue;
>        }
> +      permissions.push(new Permission(permissions.length, 
> +									  nextPermission.principal, host,

1) Please remove the trailing space in the first line,
2) Please no tab characters, use spaces to indent instead,
3) Please put host on its own line.
Attachment #8672371 - Flags: feedback+

Comment 22

4 years ago
May I recommend to limit this one to Bugs introduced with Gecko 42? AFAIK we have several problems because SM has not been adapted to some changes in SM 42, but it seems  improbable that Bugs what appeared with SM 2.38 (Based on Gecko 41) are DUPs of this one.
User Story: (updated)
Version: Trunk → SeaMonkey 2.39 Branch
Assignee

Comment 23

4 years ago
Robert,

thanks for the comments. I am not that familar with the whole process and I wasn't and still aren't sure that this is a final patch. I did test it only with cookies and general permissions and it seems to work fine here with x64 Windows 2.38 and 2.41. Maybe someone else can check out if it's working with passwords and local storage too. 

I left the rawHost replace in but I think you are right and it can be removed. Just didn't want to mess with things I only half understand.

Hopefully all tabs and trailing spaces should be gone this time.

I removed all my old permissions and the duplicate entries seem to be gone and no new ones appear. I think this might be another bug. Maybe the old profile needs somehow to be migrated for 2.39 too.
Attachment #8672371 - Attachment is obsolete: true
Assignee

Comment 24

4 years ago
>> x64 Windows 2.38 

Opps I mean 2.39 of course.
Assignee

Comment 25

4 years ago
Comment on attachment 8672401 [details] [diff] [review]
1188348-permission-V3.patch

[Approval Request Comment]
Regression caused by (bug #): 1173523
User impact if declined: Data Manager and Cookie Viewer will not work
Testing completed (on m-c, etc.):
Risk to taking this patch (and alternatives if risky): it's already broken so no risk
String changes made by this patch:
Attachment #8672401 - Flags: review?(kairo)
Attachment #8672401 - Flags: approval-comm-beta?
Attachment #8672401 - Flags: approval-comm-aurora?
Assignee

Comment 26

4 years ago
The replace with the regular expression needs to stay in the code. Tried to remove it yesterday and got unexpected results (leading point was shown in domain name). The duplicate cookie entries are imho another bug but I didn't have time to check the actual data items for differences.
As a workaround, the following URLs are still working for the time being (tested in 2015-10-08 2.41a1 nightly):
• chrome://communicator/content/permissions/cookieViewer.xul (old-style cookie manager)
• chrome://communicator/content/permissions/permissionsManager.xul (old-style permissions manager)
• chrome://passwordmgr/content/passwordManager.xul (Firefox-style password manager)
Whiteboard: [workaround see comment #28]
Assignee

Comment 29

4 years ago
>> As a workaround, the following URLs are still working for the time being (tested in 2015-10-08 2.41a1 nightly):

Are you really sure? 

This bug also covers the cookieviwer breakage. I didn't look into passwordManager yet because I don't use it but if it uses the old permission api it should be broken too.  

PermissionsManager is broken since 2.13. See Bug 1209626. I tried to fix it too but my knowledge of Javascript and Mozilla apis in general is too limited for this one.
(In reply to Frank-Rainer Grahl from comment #29)
> >> As a workaround, the following URLs are still working for the time being (tested in 2015-10-08 2.41a1 nightly):
> 
> Are you really sure? 
> 
> This bug also covers the cookieviwer breakage. I didn't look into
> passwordManager yet because I don't use it but if it uses the old permission
> api it should be broken too.  
> 
> PermissionsManager is broken since 2.13. See Bug 1209626. I tried to fix it
> too but my knowledge of Javascript and Mozilla apis in general is too
> limited for this one.

You're right, the Permissions Manager is empty, and when I think of it, I have some permissions set: many sites have cookies either allowed, blocked, or reduce-to-session; and a few sites (such as AMO and Mozdev) are whitelisted for addon-install. I cannot edit a past comment so let's reformulate it:

As a workaround, the following URLs are still working for the time being (tested in 2015-10-08 2.41a1 nightly):
• chrome://communicator/content/permissions/cookieViewer.xul (old-style cookie manager)
• chrome://passwordmgr/content/passwordManager.xul (Firefox-style password manager)

No workaround discovered so far for the Permissions Manager.
Whiteboard: [workaround see comment #28] → [workaround see comment #30]
Assignee

Comment 31

4 years ago
Merge day comes near. Anything I still need to do to make the patch ready to go into the repositories?

Thanks

Comment 32

4 years ago
I'm trying to get to reviewing it but I'm pretty busy at my day job and in addition had to figure out some build issues on nightly that stopped me from even trying and testing any patch for SeaMonkey - but I'm getting there, I hope this weekend.
Assignee

Comment 33

4 years ago
Thanks. No problem. We all have day jobs and I fully understand. In the next life I aim for billionaire :) Just wasn't sure if anything else is needed.

FRG

Comment 34

4 years ago
There is almost the same patch (for dataman) in Bug 1180871
See Also: → 1180871
Assignee

Comment 35

4 years ago
>> There is almost the same patch (for dataman) in Bug 1180871

The one there looks a lot more complete than this one. Except for the cookieviewer mine should be scrapped in favor of the other one.

Comment 36

4 years ago
Comment on attachment 8672401 [details] [diff] [review]
1188348-permission-V3.patch

So, this patch mostly works fine, improves things a whole lot for sure.

But when I try to adjust permissions to a different setting (Allow/Block), I get this:

Error: TypeError: permElem is null
Source file: chrome://communicator/content/dataman/dataman.js
Line: 1510


And when I try to reset it to default I get:

Error: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsIPermissionManager.remove]
Source file: chrome://communicator/content/dataman/dataman.xml
Line: 75

Comment 37

4 years ago
The latter error I also get when trying to add a permission.

Comment 38

4 years ago
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #36)
> Error: TypeError: permElem is null
> Source file: chrome://communicator/content/dataman/dataman.js
> Line: 1510

Line 1481 has this:

          if (elem.getAttribute("host") == aSubject.host &&
              elem.getAttribute("type") == aSubject.type)
            permElem = elem;

That .host probably doesn't exist any more, we may just get the principal here.


> Error: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument
> arg 0 [nsIPermissionManager.remove]
> Source file: chrome://communicator/content/dataman/dataman.xml
> Line: 75

That line is:

              Services.perms.remove(this.host, this.type);

This also probably doesn't work with a host any more but with a principal.

Comment 39

4 years ago
Neil, should we take this patch (with fixes) here, or yours in bug 1180871? If the latter, are you incorporating the pieces that are here and not there?
Flags: needinfo?(neil)

Comment 40

4 years ago
Comment on attachment 8672401 [details] [diff] [review]
1188348-permission-V3.patch

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

OK, I will give you a feedback+ because this is clearly the right approach, thanks a lot for this work!
Don't take it too hard, but as my errors show, there are still some things the patch needs to cover, so I will not grant you an actual review yet. Would be awesome if you could cover those pieces as well!

Removing the uplift requests for now as the patch isn't ready just yet.

Let's also see what Neil says, though.
Attachment #8672401 - Flags: review?(kairo)
Attachment #8672401 - Flags: review-
Attachment #8672401 - Flags: feedback+
Attachment #8672401 - Flags: approval-comm-beta?
Attachment #8672401 - Flags: approval-comm-aurora?
(In reply to Robert Kaiser from comment #39)
> Neil, should we take this patch (with fixes) here, or yours in bug 1180871?
> If the latter, are you incorporating the pieces that are here and not there?

My patch doesn't address comment #36 either, except it has a workaround for resetting permissions (not as good as Frank-Ranier's code for removing permissions though).

(In reply to Frank-Rainer Grahl from comment #35)
> The one there looks a lot more complete than this one.

Well, you were only working on the data manager, while I was working on all the calls to nsIPermissionManager::Remove that I could find. Since you've got so far here though it's probably better for you to finish it off (if you think you can).
Flags: needinfo?(neil)
Assignee

Comment 42

4 years ago
Thanks you two for the review and the comments. I missed the permElem is null because the code worked and changed/removed the permission.

I am busy over the week but can look into it this weekend. My knowledge of this stuff is very very limited but I should be able the fix the remaining dataman issues. If not I will raise the white flag. I think this should be fixed for a 2.39 beta so if I take too long just let me know or do it yourself. I will not object.

Happy week
FRG

Comment 43

4 years ago
You all are missing the problem. Domains with passwords *ARE* listed. Its just that the Password tab is greyed out, inactive, inaccessible.

Comment 44

4 years ago
(In reply to James B. Higgs from comment #43)
> You all are missing the problem. Domains with passwords *ARE* listed. Its
> just that the Password tab is greyed out, inactive, inaccessible.

The same is true for Permissions tab, Preferences tab, etc.

Comment 45

4 years ago
James, we know. The Data Manager is broken, it just lists domains but you can't do anything with them, and the patch here fixes that successfully. There is some remaining issues with managing permissions not working correctly even with the patch and that still needs to be fixed, Frank-Rainer is looking into that.

Comment 46

4 years ago
Comment on attachment 8672401 [details] [diff] [review]
1188348-permission-V3.patch

Actually, after discussion on IRC, I'll mark this r+ so we can take at least that part of the fix for beta 1, but let's get a patch for the additional pieces for release if possible.
Attachment #8672401 - Flags: review-
Attachment #8672401 - Flags: review+
Attachment #8672401 - Flags: feedback+
Comment on attachment 8672401 [details] [diff] [review]
1188348-permission-V3.patch

[Triage Comment]
This goes into comm-beta.
Attachment #8672401 - Flags: approval-comm-beta+
Comment on attachment 8672401 [details] [diff] [review]
1188348-permission-V3.patch

[Triage Comment]
As per convo with Callek over irc,  this needs to go to c-a as well.
Attachment #8672401 - Flags: approval-comm-aurora+
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Comment on attachment 8672401 [details] [diff] [review]
1188348-permission-V3.patch

Pushed to comm-central:
  https://hg.mozilla.org/comm-central/rev/9925b16e6c6c
Assignee

Comment 51

4 years ago
If you want the remaing fixes fast I had a V4 ready tuesday evening which fixes all exceptions and where basically everything worked but still experienced some wierd results adding hosts and domains manually. Need to compare this to 2.38 first.
(In reply to Edmund Wong (:ewong) from comment #50)
> Comment on attachment 8672401 [details] [diff] [review]
> 1188348-permission-V3.patch
> 
> Pushed to comm-central:
>   https://hg.mozilla.org/comm-central/rev/9925b16e6c6c

Data Manager is functional again on:

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0 SeaMonkey/2.41a1" ID:20151029003002 c-c:9925b16e6c6c2bbca8eccd189e6962b6bdb6c214 m-c:1e700005a0ddf2b17803213e1f3f8d78a7a618b8 en-US

On yesterday's nightly it still wasn't.
Assignee

Comment 53

4 years ago
The part 2 patch should fix the remaining issues in data manager. Basically it only needed the fixes for comment #36. I took the dataman.xml patch 1:1 from Neils first patch in Bug 1180871.

The data manager still behaves a little strange when you manually add permissions. If you do not change the defaults it will not add them afterwards and it will also not refresh the left list. But this behaviour was also present in 2.38. I think this part might need an overhaul soon anyway because some apis are deprecated now but this is over my head right now. If you find any further errors let me know and I will try to fix them asap.

If the fix is ok it should be pushed to all trees. It applied clean to my release and central tree.
Attachment #8681343 - Flags: review?
Assignee

Comment 54

4 years ago
oops mangled the patch name
Attachment #8681343 - Attachment is obsolete: true
Attachment #8681343 - Flags: review?
Attachment #8681345 - Flags: review?

Comment 55

4 years ago
Comment on attachment 8681345 [details] [diff] [review]
1188348-permission-part2-V1.patch

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

I probably will not get to fully review this myself for some time, as I'm traveling the next 10 days. If someone like Ian or Neal steals the review away, I'm happy with that, otherwise I'll get back to it when I'm back, I hope.
The changes here look good to me in principle (with that one open question), so I'll put an f+ on it.

::: suite/common/dataman/dataman.xml
@@ +71,5 @@
>                                                                     "radioGroup");
>            radioGroup.disabled = aChecked;
>            if (aChecked) {
>              if (!aUIUpdateOnly)
> +              Services.perms.remove(Services.io.newURI("http://" + this.host, null, null), this.type);

Is this always http? never https?
Attachment #8681345 - Flags: review?(kairo)
Attachment #8681345 - Flags: review?
Attachment #8681345 - Flags: feedback+
Assignee

Comment 56

4 years ago
>> Is this always http? never https?

Seriously I can't tell. I took this straight from Neils patch and tested it on a few values in my data manager. It would be better to use the pricipal but I am not sure this is available in the xml without some refactoring.

If I can put addition items in the list which are not shown then using the principal would be easy.

FRG
(In reply to Tony Mechelynck [:tonymec] from comment #52)
> (In reply to Edmund Wong (:ewong) from comment #50)
> > Comment on attachment 8672401 [details] [diff] [review]
> > 1188348-permission-V3.patch
> > 
> > Pushed to comm-central:
> >   https://hg.mozilla.org/comm-central/rev/9925b16e6c6c
> 
> Data Manager is functional again on:
> 
> UA:"Mozilla/5.0 (X11; Linux x86_64; rv:44.0) Gecko/20100101 Firefox/44.0
> SeaMonkey/2.41a1" ID:20151029003002
> c-c:9925b16e6c6c2bbca8eccd189e6962b6bdb6c214
> m-c:1e700005a0ddf2b17803213e1f3f8d78a7a618b8 en-US
> 
> On yesterday's nightly it still wasn't.

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0 SeaMonkey/2.42a1" ID:20151030003002 c-c:290ae28c7e983753e2c0e9cceb124ae0b3207996 m-c:b41b92c09fcf94d077a54297aea1dc675b161a9d en-US

After further testing it is still not fully operational though. At least the tabs are not all greyed-out anymore; but the old-style chrome URLs in comment #30 are still useful.
Assignee

Comment 58

4 years ago
Tony please check if you experience the problems described in comment #36. These are fixed with the second patch. If you see other javascript errors please post them here.
Flags: needinfo?(antoine.mechelynck)
(In reply to Frank-Rainer Grahl from comment #58)
> Tony please check if you experience the problems described in comment #36.
> These are fixed with the second patch. If you see other javascript errors
> please post them here.

UA:"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0 SeaMonkey/2.42a1" ID:20151031003002 c-c:2ef88c6fc965556e7c1a19e97b53222b47a0dc78 m-c:c1a94d5365e48f86de6a7bb6a3c4c7d82b9d660e en-US

When a specific domain is selected, all relevant Data Manager tabs are active and contain information. For domain "*", the "Permissions" tab is not greyed-out but it is empty; the "Forms Data tab (which may be due to an extension) seems functional; all other tabs are greyed-out. Displaying all passwords for a given site is possible again, which is new AFAICT. Displaying cookies also works. Displaying specific-site permissions works, but if I try to change a set-cookie permission from "Allow for Session" to "Allow" then reload the data manager (by clicking the Reload button in the browser chrome), I see that my change has not taken hold. I haven't tried to remove cookies, permissions or passwords.

Nothing in the Error Console (with Console² 0.9.1 installed and Errors, Warnings and Messages all selected).
Flags: needinfo?(antoine.mechelynck)

Comment 60

4 years ago
(In reply to Tony Mechelynck [:tonymec] from comment #59)
> When a specific domain is selected, all relevant Data Manager tabs are
> active and contain information. For domain "*", the "Permissions" tab is not
> greyed-out but it is empty; the "Forms Data tab (which may be due to an
> extension) seems functional; all other tabs are greyed-out.

That is correct and expected behavior. "*" is not a real domain, it's an entry that is only there because Form Data is not saved by domain but just in general. And we had to enable the empty Permissions tab there so you are actually able to add a permission for a domain that is not listed (yet).


> but if I try to change a set-cookie permission from "Allow for Session" to
> "Allow" then reload the data manager (by clicking the Reload button in the
> browser chrome), I see that my change has not taken hold.

That's one of the errors that the additional patch should fix when it lands.
Assignee

Comment 61

4 years ago
>> That's one of the errors that the additional patch should fix when it lands.

Just tested again against 2.38. It does but seems to double the permissons with https hosts. 

I think the call to gDomains.hostMatchesSelected(rawHost) in line 1461 returns false and then a permission is added instead of replacing it. Happens only with the first entry in the list for this domain. Need to check this. Can be verified with google and works fine in 2.38. 

FRG
Assignee

Comment 62

4 years ago
Ok,

found it. Robert your remark in 55 did hit the nail. Dataman.xml needs to be changed a lot more. All parts where a http:// is added in front of the host need to change if an existing permission is added, changed or removed. 

Line 1501 permElem.setCapability(aSubject.capability, true) seems to be the problem.

Not sure if I can fix this fast.

FRG

Comment 63

4 years ago
Yes, I think that the reason why permissions were switched to principals instead of hosts was that http and https should be treated differently.
(In reply to Tony Mechelynck [:tonymec] from comment #59)
[...]
> Nothing in the Error Console (with Console² 0.9.1 installed and Errors,
> Warnings and Messages all selected).

OTOH, with Console² disabled the Error Console display scrolls so fast that I don't have time to see what it is about.
Assignee

Comment 65

4 years ago
Ok, this is over my head and might be an Observer or cookie genration problem with https hosts. The code works always on the second to last item in the permissions panel. I am able to reproduce this with google.com. I have not seen it with http:// hosts.

To reproduce:

Remove all permissions for google.com. Diplay google.com so that a cookie permissions entry is generated. I have set cookies to always ask and set them to be only good for the current session.

Now open the data manager and change the entry. If you enable debug messages an 'Observed: perm-changed - added' will be displayed in the error console. This is wrong because we change an existing entry. This does not occur with 2.38.

If you remove all permissions and manually add another permission e.g. for images first you will see that the cookie permission is the second one in the panel. The observer works correctly in this case and you see a perms-changed message. 

Can make screenshots to show the problem.
Assignee

Comment 66

4 years ago
I just stumbled over 2 additional bugs in Data Manager. This time in the storage section. 

I checked the patches using the tests on the follwing page:

http://dev-test.nemikor.com/web-storage/support-test/

I think the size item in the storage panel can be removed now. It's always set to 0 now. Or we keep it for future use?

Besides from the known bug with the first permission on the panel which I think is an Observer problem the code seems to be stable. I used it a lot in the last weeks and didn't notice any additional errors. 2.40 is near and it might be a good idea to put it into the repositories.
Attachment #8681345 - Attachment is obsolete: true
Attachment #8681345 - Flags: review?(kairo)
Attachment #8693239 - Flags: review?(kairo)
Assignee

Comment 68

4 years ago
Posted file Do it yourself fix (obsolete) —
If you want to fix dataman yourself until this is checked in download the dataman.zip file. It cointains the fixed versions of dataman.js and dataman.xml.

Instructions for Windows:

Copy omni.ja (see omni.jpg) from your Seamonkey program directory to a safe location. 

Copy or rename it to omni.zip. 

Locate the old files in the zip. See zip folder output in omni2.jpg. They are in chrome\comm\content\communicator\dataman.

Replace dataman.js and dataman.xml with the fixed versions contained in the zip file.

Rename omni.zip back to omni.ja and copy it back. When you open Seamonkey you might need to clean your cache if the fixed versions are not picked up. 

If something goes wrong with the patched omni.ja you need to reinstall Seamonkey so might want to keep a backup.

This works on Linux too. These files are not Windows specific. You just need to locate the path of the Seamonkey installation files there (might be under opt/seamonkey or somewhere under /usr).
Assignee

Updated

4 years ago
Whiteboard: [workaround see comment #30] → [workaround see comments #30 and #68]

Comment 69

4 years ago
I am afraid the fix mentioned in comment#68 didn't work for me. Files in omni.ja are replaced, seamonkey restarted, disk cache is not used and yet passwords are still invisible.

$unzip -l omni.ja |grep -E "dataman\.(js|xml)"
   119517  11-28-2015 19:53   chrome/comm/content/communicator/dataman/dataman.js
    10923  11-28-2015 19:53   chrome/comm/content/communicator/dataman/dataman.xml
Assignee

Comment 70

4 years ago
Do you see any javascript Errors in the console? I checked it with my private comm-central build and it works with passwords.

Any password extensions installed?

Comment 71

4 years ago
No password-related extensions, and there are errors - this is the result of opening about:data

Timestamp: 08-12-15 09:51:07
Error: DEPRECATION WARNING: nsIContentPrefService is deprecated. Please use nsIContentPrefService2 instead.
You may find more details about this deprecation at: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIContentPrefService2
chrome://communicator/content/dataman/dataman.js 60 dataman_initialize
about:data 1 onload
null 0 null

Source File: resource://gre/modules/Deprecated.jsm
Line: 79

Timestamp: 08-12-15 09:51:07
Error: DEPRECATION WARNING: nsIContentPrefService is deprecated. Please use nsIContentPrefService2 instead.
You may find more details about this deprecation at: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIContentPrefService2
chrome://communicator/content/dataman/dataman.js 261 domain_initialize
chrome://communicator/content/dataman/dataman.js 69 dataman_initialize
about:data 1 onload
null 0 null

Source File: resource://gre/modules/Deprecated.jsm
Line: 79

Timestamp: 08-12-15 09:51:07
Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host]
Source File: chrome://communicator/content/dataman/dataman.js
Line: 288
Assignee

Comment 72

4 years ago
The first two are normal. The third one might be the problem.

Could you add a data_manager.debug boolean variable in about:config and set it to true. The addition output should show the offending host or domain. Does the old password manager work?

I unfortunately can not reproduce it for the life of it.
(In reply to Frank-Rainer Grahl from comment #72)
> The first two are normal. The third one might be the problem.
> 
> Could you add a data_manager.debug boolean variable in about:config and set
> it to true. The addition output should show the offending host or domain.
> Does the old password manager work?
> 
> I unfortunately can not reproduce it for the life of it.

To get a password manager like Firefox's (not like SeaMonkey used to have before the Data Manager existed, but like what a modern Firefox version built on the same Gecko/Toolkit version would have), browse to chrome://passwordmgr/content/passwordManager.xul

Comment 74

4 years ago
(In reply to Frank-Rainer Grahl from comment #72)
> The first two are normal. The third one might be the problem.
> 
> Could you add a data_manager.debug boolean variable in about:config and set
> it to true. The addition output should show the offending host or domain.

There are two types of errors: fixed IP addresses and insufficient domain levels.

Timestamp: 08-12-15 15:21:17
Error: Error while trying to get domain from host name: hd
Source File: chrome://communicator/content/dataman/dataman.js
Line: 118

Timestamp: 08-12-15 15:21:17
Error: [Exception... "Component returned failure code: 0x804b0050 (NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) [nsIEffectiveTLDService.getBaseDomainFromHost]"  nsresult: "0x804b0050 (NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS)"  location: "JS frame :: chrome://communicator/content/dataman/dataman.js :: domain_getDomainFromHost :: line 494"  data: no]
Source File: chrome://communicator/content/dataman/dataman.js
Line: 118

cached: hd -> hd

[skipped]

Timestamp: 08-12-15 15:21:17
Error: Error while trying to get domain from host name: 11.206.0.160
Source File: chrome://communicator/content/dataman/dataman.js
Line: 118

Timestamp: 08-12-15 15:21:17
Error: [Exception... "Component returned failure code: 0x804b0051 (NS_ERROR_HOST_IS_IP_ADDRESS) [nsIEffectiveTLDService.getBaseDomainFromHost]"  nsresult: "0x804b0051 (NS_ERROR_HOST_IS_IP_ADDRESS)"  location: "JS frame :: chrome://communicator/content/dataman/dataman.js :: domain_getDomainFromHost :: line 494"  data: no]
Source File: chrome://communicator/content/dataman/dataman.js
Line: 118

cached: 11.206.0.160 -> 11.206.0.160

[skipped. And it all ends with]

Timestamp: 08-12-15 15:21:17
Error: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.host]
Source File: chrome://communicator/content/dataman/dataman.js
Line: 288

Comment 75

4 years ago
Comment on attachment 8693239 [details] [diff] [review]
1188348-permission-part2-V2.patch

> +    let type = aData;
> +    
> +    // session storage also comes here, but currently not supported
> +    // aData: null, all data in aSubject
> +    // see https://developer.mozilla.org/en/DOM/Event/StorageEvent
> +    if (type != "localStorage" && 
> +        type != "sessionStorage") {
> +       Components.utils.reportError("Observed an unrecognized storage change of type " + type);

I think this would be clearer if written as:

switch (aData) {
  case "localStorage":
  case "sessionStorage":
    break;
  default
    Components.utils.reportError("Observed an unrecognized storage change of type " + aData);
}
Attachment #8693239 - Flags: review?(kairo) → review?(neil)
Assignee

Comment 76

4 years ago
Yupp looks better with a case.

And the name of patch part 2 V3 is patch part 2 V3 :)

lvm I will look into hosts without domains and ip adresses but these messages should be non fatal. It's also generated for *
Attachment #8693239 - Attachment is obsolete: true
Attachment #8693239 - Flags: review?(neil)
Attachment #8696687 - Flags: review?(neil)
Assignee

Comment 77

4 years ago
Comment on attachment 8696687 [details] [diff] [review]
1188348-permission-part2-V3.patch

patch is bad plase do not check in. Fix coming soon.
Attachment #8696687 - Flags: review?(neil)
Assignee

Comment 78

4 years ago
Typo corrected. Sorry
Attachment #8696687 - Attachment is obsolete: true
Attachment #8696694 - Flags: review?(neil)

Comment 79

4 years ago
(In reply to Frank-Rainer Grahl from comment #76)
> lvm I will look into hosts without domains and ip adresses but these
> messages should be non fatal. It's also generated for *

In my case there are no messages for *, only for "real" IP and non-FQDN records even though record for * exists (form data only).
Assignee

Comment 80

4 years ago
I can't reproduce it but I am unable to test any passwords with IP or host only addresses. Would need to install a web server in my local net and presently not enough time. I can not reproduce it with domain passwords. Even the unfixed version shows them. 

Did you try a new profile? I recently had some weird repainting and also permission problems and they did only go away after I nuked my old profile. 

Btw. removing forms data is currently no longer possible in nightly. Appconstants not found. See Bug 1160782 but maybe still Data Manager or Build Config problem.

Comment 81

4 years ago
(In reply to Frank-Rainer Grahl from comment #80)
> I can't reproduce it but I am unable to test any passwords with IP or host
> only addresses. Would need to install a web server in my local net and
> presently not enough time.

I might be wrong, but I don't think you need the actual hosts, just the records in logins.json which can be created manually.

Comment 82

4 years ago
Comment on attachment 8696694 [details] [diff] [review]
1188348-permission-part2-V4.patch

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

::: suite/common/dataman/dataman.xml
@@ +71,5 @@
>                                                                     "radioGroup");
>            radioGroup.disabled = aChecked;
>            if (aChecked) {
>              if (!aUIUpdateOnly)
> +              Services.perms.remove(Services.io.newURI("http://" + this.host, null, null), this.type);

Unfortunately I think this is not enough and just a bad workaround. We can't know that http is the right scheme, it could just as well be https.

Also, for the case of the add a permission UI, I think we probably need to add a selector for the scheme in the UI or make sure that either people enter the scheme with the host in the UI, or add it (there we can default to http).

Comment 83

4 years ago
Comment on attachment 8696694 [details] [diff] [review]
1188348-permission-part2-V4.patch

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

::: suite/common/dataman/dataman.js
@@ +2347,5 @@
> +                              // But I think it's not worth it. Seems the only way 
> +                              // to do this is to get all the key names and values
> +                              // and add the string lengths together
> +                              // size: gLocSvc.domstoremgr.getUsage(rawHost),
> +                              size: 0,

Please do not conflate the permissions part this bug should be about and the web storage part that bug 1045989 is about. Please do separate patches for both. IMHO, we just should remove the usage column completely. I actually wonder if we should remove the whole web storage panel as it never worked without issues due to web storages not supporting a manager interface decently at all.
Attachment #8696694 - Flags: feedback-
Assignee

Comment 84

4 years ago
Posted image Capture.PNG
Bug is not forgotten: I just didn't really have much time. I fixed the old Cookie Manager first to see what needs to be done. Still needs some minor work but looks promising. Scheme and port are displayed and a full url can be entered. Just needs some additional validation and then I will port it to Data Manager.
Assignee

Comment 85

4 years ago
Hi Robert,

I got Data Manager so far that it now displays the scheme and if present the port by displaying the origin instead of the host. It removes and updates the permissions correctly. Add is next. 

A few questions to you or anyone who reads this:

- Should the host password permissions get a new page? Loginmanager still uses a host and not a principal and the code would likely be getting spaghetti-ish with ifs and else all over the place. I just left this code out for the tests and need to reintegrate it.

- Does anyone know and IDN domain which does not redirect you to a plain ascii one?

I noticed that when you have displayed a different tab than permission before or the first time the panel opens it's initilized twice. Not exactly a bug and should alredy be so in the current implementation. Room for improvement later.

FRG
Attachment #8703329 - Flags: feedback?(kairo)
Assignee

Comment 86

4 years ago
Ok, I think I have fixed it for good.

There is still room for improvement eg. Password Permissions uses only one item in the add listbox which need to be selected. It's more or less a plain copy of permissions.

Not sure about IDN domains. They should display ok as xn-- domains. I didn't find one which didn't immediately redirected me to an ascii domain. 

I would prefer that the storage fixes stay in. S*cks for testing to have another patchset.

I did put some debug messages in. They are only conditionally enabled and helped me greatly to understand the flow of the program. Should they be removed?

FRG
Attachment #8696694 - Attachment is obsolete: true
Attachment #8696694 - Flags: review?(neil)
Attachment #8705383 - Flags: review?(kairo)
Assignee

Comment 87

4 years ago
String changes for de locale.
Attachment #8705384 - Flags: review?(kairo)

Updated

4 years ago
Attachment #8703329 - Flags: feedback?(kairo) → feedback+

Comment 88

4 years ago
Comment on attachment 8705383 [details] [diff] [review]
1188348-permission-part2-V5.patch

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

Overall, I find myself losing interest in doing lengthy reviews on this code, forwarding to Neil, I'll remove myself from ownership here.

All that said, thanks for digging into this, it's very very appreciated, I personally just am probably not the right person for this any more.

::: suite/common/dataman/dataman.css
@@ +47,5 @@
> +/* password item has different labels */
> +richlistitem.permissionpwd[type="password"] {
> +  -moz-binding: url('chrome://communicator/content/dataman/dataman.xml#permpwd-base-item');
> +  -moz-box-orient: vertical;
> +}

Why are you moving this? I think it should stay where it is.
Attachment #8705383 - Flags: review?(kairo) → review?(neil)

Comment 89

4 years ago
Comment on attachment 8705384 [details] [diff] [review]
1188348-permission-part2-V5-de.patch

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

Please don't attach patches for a localization to a general bug, this should be left to the localizers. A lot of it is probably good, but I am using tools that don't work with patches (but produce ones), so I need to redo it anyhow.
Attachment #8705384 - Flags: review?(kairo) → review-
Assignee

Comment 90

4 years ago
>> Why are you moving this? I think it should stay where it is.

The password api is completely different form the permissions api and uses different notifications too. Now that the permissions api also no longer uses a simple host, the code to handle it would get more complicated. I felt that separting it was the better choice despite some duplication. The other parts especially handling notifications are much simpler now because of this.

I was unsure how to handle the localization. I will keep it in mind to not do it myself. 

Thanks
FRG
Assignee

Comment 91

4 years ago
Comment on attachment 8705384 [details] [diff] [review]
1188348-permission-part2-V5-de.patch

de locale patch contained some goofs. Is no good in current state and was not supposed to be in this bug so obsoleted it.
Attachment #8705384 - Attachment is obsolete: true
Assignee

Comment 92

4 years ago
Posted file DatamanV2.zip
Do it yourself fix Verson 2

While the review process is going on you might want to become a beta tester and fix it yourself. It is also unlikely that 2.40 will include the fixes. 
Download the datamanV2.zip file. It contains the fixed Data Manager and old Cookie Manager versions and should work for 2.39 too.

Instructions for Windows:

Copy omni.ja (see omni.jpg) from your Seamonkey program directory to a safe location.

Copy or rename it to omni.zip. 

Use your favorite archive manager (Windows Explorer will do too) and locate the old files in the zip. The locations can be found in the omni.txt files which can be found in the zip. They are in the dataman and cookieviewer directories.

Replace the files with the fixed versions contained in the zip file. I provided de and en-us translations. 
For other languages you might want to use the en-us files and copy them to the appropriate locale directories but you are on your own with this. 

Rename omni.zip back to omni.ja and copy it back. When you open Seamonkey you might need to clean your cache if the fixed versions are not picked up. 

If something goes wrong with the patched omni.ja you need to reinstall Seamonkey so you might want to keep a backup.

This works on Linux too. These files are not Windows specific. You just need to locate the path of the Seamonkey installation files there (might be under opt/seamonkey or somewhere under /usr).

If it works for you please leave feedback if something does not seem to be right so that it can be fixed soon. I only did limited testing with non http and https permissions and couldn't find an IDN website for testing.

And remember: If your computer blows up because of it it's your fault not mine :)

FRG
Attachment #8696218 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Whiteboard: [workaround see comments #30 and #68] → [workaround see comments #30 and #92]

Updated

4 years ago
Attachment #8705383 - Flags: superreview?(iann_bugzilla)

Updated

4 years ago
Blocks: 1240738

Comment 93

3 years ago
Comment on attachment 8705383 [details] [diff] [review]
1188348-permission-part2-V5.patch

The following is just a code correctness check. I don't pretend to understand this code.
Also I haven't looked at the cookie code yet. Perhaps a separate patch/bug would be in order.

> +    elemLoop: for (let i = 0; i < this.listPermission.length; i++) {
...
> +        break elemLoop;

Don't use a label. It is totally unnecessary.
Use for ... of e.g.
  for (let element of this.listPermission) {...}
Consider using Map()

Inline the return value:
+        permElem = elem
+        break elemLoop;
.........return elem;

+      }
+    }
+    return permElem;
.....return null;

> +    // This happens when we add a new permission
> +    if (permElem == null) {
> +      gDataman.debugMsg("Unable to find an Item: " + aOrigin + " " + aType);
> +    }
> +    else {

Use an early return and don't use an else after the return e.g.
  if (!permElem)
    return;

  gDataman.debugMsg("Found Element " + permElem.origin);

> +        try {
> +          gDataman.debugMsg("Removing permission");
> +          Services.perms.removeFromPrincipal(permElem.principal, permElem.type);
Why do you think this will throw?

>         // This should all not fail because origin and type and capability should
> +        // all have been checked at least once before. But better safe than sorry.
> +        try {
Don't bother with try/catch here.

> +        // update permission. There is no direct update function available.
> +        // The add will update the existing permission.
> +        // Any further actions will be done in the code dealing with the subsequent observer message.
> +        try {
Or here.

> +      // let the backend do the validation of the input field
> +      let nOrigin = "";
> +
> +      try {
> +        nOrigin = new URL(this.addHost.value).origin;
> +      } catch (e) {
> +        // show an error if URL is invalid
> +        window.alert(gDataman.bundle.getString("perm.validation.invalidurl"));
> +        return;
> +      }
> +
> +      // url could be validated but User did probably enter half valid nonsense
> +      // becuse the origin is undefined
> +      if ((nOrigin == null) || (nOrigin == "")) {
> +        window.alert(gDataman.bundle.getString("perm.validation.invalidurl"));
> +        return;
> +      }

Please don't use modal alerts here.

      let nOrigin;
      try {
        nOrigin = new URL(this.addHost.value).origin;
      } catch (e) {}

      if (!nOrigin) {
        gDataman.debugMsg(...);
        return;
      }

> +      for (let i = 0; i < this.list.children.length; i++) {
> +        let elem = this.list.children[i];
for (let elem of this.list.children) { ....

> +        let enumerator = Services.perms.enumerator;
> +        while (enumerator.hasMoreElements()) {
> +          let nextPermission = enumerator.getNext();
> +          nextPermission = nextPermission.QueryInterface(Components.interfaces.nsIPermission);
> +          if (domain == gDomains.getDomainFromHost(nextPermission.principal.URI.host.replace(/^\./, "")))
> +            haveDomainPerms = true;
Optimize: add a break; here.
> +        }
> +        if (!haveDomainPerms)

> +    for (let i = 0; i < rejectHosts.length; i++) {
> +      if (gDomains.hostMatchesSelected(rejectHosts[i])) {
Use for...of

> +      for (let i = 0; i < this.list.children.length; i++) {
> +        let elem = this.list.children[i];
Use for...of

> +  <binding id="permpwd-base-item"
> +           extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
Consider extending "perm-base-item" instead.

You have two properties called "useDefault" which one do you want?
Attachment #8705383 - Flags: feedback-
Assignee

Comment 94

3 years ago
Ratty,

thanks for the feedback. Will go over it. A few nitpicks

>> Use an early return and don't use an else after the return e.g.

Will do. There seem to be 2 fractions: one say only one return. Second use as many as you wish. I am usually favor of type 1 unless it's because of errors where I need to do an endless row of 'if not bad ... if not bad ... if not bad ...  if not'. Also exceptions do apply to my rule :)

>> > +      // url could be validated but User did probably enter half valid nonsense
>> > +      // becuse the origin is undefined
>> > +      if ((nOrigin == null) || (nOrigin == "")) {
>> > +        window.alert(gDataman.bundle.getString("perm.validation.invalidurl"));
>> > +        return;
>> > +      }

>> Please don't use modal alerts here.

What would you propose here? The window is shown because a direct user interaction did go wrong. Most likely a garbled url was entered. Message in the status bar might be overlooked or it might be even hidden. 

>> > +  <binding id="permpwd-base-item"
>> > +           extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>> Consider extending "perm-base-item" instead.

This was how it was but I wanted the password permissions to be separate entities because they use a different api.

>> You have two properties called "useDefault" which one do you want?

Need to check

FRG
Flags: needinfo?(philip.chee)
@FRG:
Instead of an alert you might want to show a doorhanger (which disappears if you click outside it and reappears if you click its icon next to the tabs's favicon at the start of the URL bar). This is just as obvious as an alert popup, but it isn't blocking. I don't know how to code it, but if I had to I would search MDN for it.

Ratty might have a better idea though.

Comment 96

3 years ago
(In reply to Tony Mechelynck [:tonymec] from comment #95)
> Instead of an alert you might want to show a doorhanger (which disappears if
> you click outside it and reappears if you click its icon next to the tabs's
> favicon at the start of the URL bar).

Doorhangers are only for things tied to a certain website, not things tied to UI.

Comment 97

3 years ago
That said, IMHO you should display things like this inside the UI and not in a warning window.

Comment 98

3 years ago
>> Use an early return and don't use an else after the return e.g.

> Will do. There seem to be 2 fractions: one say only one return. Second use 
> as many as you wish. I am usually favor of type 1 unless it's because of 
> errors where I need to do an endless row of 'if not bad ... if not bad ... 
> if not bad ...  if not'. Also exceptions do apply to my rule :)

The early return I mentioned applies to a specific case:

if (foo) {
  ...
  return
}
else {
  do something
  ...
}

In this case you can unwrap the else clause:

if (foo) {
  ...
  return
}

do something
...etc.

>> Please don't use modal alerts here.

> What would you propose here? The window is shown because a direct user 
> interaction did go wrong. Most likely a garbled url was entered. Message in 
> the status bar might be overlooked or it might be even hidden. 

I would say leaving it as a modal alert temporarily to get moving on reviews but consider using a <notificationbox>

>>> +  <binding id="permpwd-base-item"
>>> +           extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>> Consider extending "perm-base-item" instead.

> This was how it was but I wanted the password permissions to be separate 
> entities because they use a different api.
OK.

You're missing some CSS for the richlist items in dataman.css

.permissionpwd, /* add this line */
.permission {
  padding-top: 6px;
  padding-bottom: 6px;
  -moz-padding-start: 7px;
  -moz-padding-end: 7px;
  min-height: 25px;
  border-bottom: 1px dotted ThreeDFace;
}
Flags: needinfo?(philip.chee)
Assignee

Comment 99

3 years ago
Thanks. I didn't have much time at hand this weekend and just got the trees building today after the merge. Will see that I hopefully wrap it up next weekend.

FRG
Assignee

Comment 100

3 years ago
And now we are at version 6. I hope I didn't overlook something from the feedback. I left 2 try/catch in the code because there are some szenarios where these might throw. 

I found another problem with permissions for about: pages which have no scheme and host and which might be responsible for some bug reports prior to 2.39.

I looked at notification box. If I find some time I will try to put this in later. This would also need some new test code but doing this is currently over my head. Not enought knowledge about the test infrastructure right now.
Attachment #8705383 - Attachment is obsolete: true
Attachment #8705383 - Flags: superreview?(iann_bugzilla)
Attachment #8705383 - Flags: review?(neil)
Attachment #8714105 - Flags: superreview?(iann_bugzilla)
Attachment #8714105 - Flags: review?(neil)
Assignee

Comment 101

3 years ago
You can currently use Ctrl-I and change permissions for about:pages in the Page Info dialog. If you do this Data Manager and Cookie Manager will be left dead in the water. This might also be the cause for passwords not displaying in Data Manager in Bug 1228737 and other problems mentioned before. I put a check in the code to ignore these. If these are really invalid they should be deleted. If not a later revision needs to fix managing them. I would rather not want to do it now:)

Comment 102

3 years ago
Comment on attachment 8714105 [details] [diff] [review]
cc-1188348-permission-part2-V6.patch

This is a very bif patch, is there no way of splitting into the permissionPwd part and then everything else?

>+++ b/suite/common/dataman/dataman.js

>+  checkValidPermforDataman: function dataman_checkValidPermforDataman(aPermission) {
Can this not just be called checkValidPermission?
>+    // it is currently possible to add a permission for about:xxx and other internal pages.
Nit: if you end a comment with a full stop (.) then you should start with a capital letter.

>+    // These are probably invalid and will be ignored for now
>+    // test if the permission has a host if not consider it invalid for now
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+      for (let rHost of rejectHosts)
>+        gDomains.addDomainOrFlag(rHost, "hasPermissionsPwd");
Not sure if host is already in use, but if not why not just use that instead of rHost?

>-      for (let i = 0; i < gPasswords.allSignons.length; i++) {
>-        gDomains.addDomainOrFlag(gPasswords.allSignons[i].hostname, "hasPasswords");
>+      for (let pHost of gPasswords.allSignons) {
>+        gDomains.addDomainOrFlag(pHost.hostname, "hasPasswords");
Is this a host or a signon?

>-      for (let i = 0; i < gStorage.storages.length; i++) {
>-        gDomains.addDomainOrFlag(gStorage.storages[i].rawHost, "hasStorage");
>+      for (let sHost of gStorage.storages) {
>+        gDomains.addDomainOrFlag(sHost.rawHost, "hasStorage");
Is this a storage rather than a host?

>-    for (let i = 0; i < fields.length; i++) {
>-      this[fields[i]].value = "";
>+    for (let i of fields) {
>+      this[i].value = "";
Should this be field instead of i?

>+        permElem.setAttribute("host", nextPermission.principal.origin);
>+        permElem.setAttribute("displayHost", nextPermission.principal.origin);
>+//                              gLocSvc.idn.convertToDisplayIDN(rawHost, {}));
Delete rather than comment out?

>+  // Find an item in the permissionsList by origin and type
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+  // Directly remove a permission
>+  // This function is called when the user checks the 'Use Default' button on the permissions panel.
>+  // The item will be removed and the default permissions for the origin will be in place afterwards.
>+  // This function will only handle the deletion. The remove will trigger an Observer message.
>+  // Because the permission might be removed outside of this panel the code in there needs to clean
>+  // up the panel and lists
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+    // This happens when we add a new permission
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+    // It might be a new element. In this case the principal is null and we do not need to do
>+    // anything here. We can not remove the list entry because it might be a new permission the
>+    // user wants to change
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+    if (permElem.principal != null) {
>+      // delete permission. We will deactivate the list item in the subsequent observer message
Nit: if you end a comment with a full stop (.) then you should start with a capital letter.
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+  // Directly change a permission
Nit: if you start a comment with a capital letter it should end with a full stop (.).
>+  // This function is called when the user changes the value of a permission on the permissions panel.
>+  // This function will only handle the update. The update will trigger an Observer message.
>+  // Because the permission might be changed outside of this panel the code in there needs to handle
>+  // further generic changes
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+    // If this is a completely new permission we do not have a principal yet.
>+    // This happens when we add a new item. We need to create a new permission
>+    // from scratch
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+      // This can currently fail for some schemes like file://
>+      // maybe fix it later if needed
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>     // this.addSelBox, this.addHost, this.addType, this.addButton
Can this be removed?

>+      // Look for a translation
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+      for (let permType of permTypes) {
>+        let typeDesc = permType;
>         try {
>+          typeDesc = gDataman.bundle.getString("perm." + permType + ".label");
>         }
>         catch (e) {
>         }
If catch is triggered what is typeDesc left as?

>+          gDomains.selectedDomain.title == "*" ? "" : ("http://www." + gDomains.selectedDomain.title);
Is it always going to be correct being http and www?

>+      let nOrigin = "";
>+
>+      try {
>+        nOrigin = new URL(this.addHost.value).origin;
>+      } catch (e) {
>+        // show an error if URL is invalid
>+        window.alert(gDataman.bundle.getString("perm.validation.invalidurl"));
>+        return;
Does this need to be in the catch, doesn't it get picked up in the if statement below?
>+      }
>+
>+      // url could be validated but User did probably enter half valid nonsense
>+      // becuse the origin is undefined
>+      if ((nOrigin == null) || (nOrigin == "")) {
>+        window.alert(gDataman.bundle.getString("perm.validation.invalidurl"));
>+        return;
>+      }


>+      // add a new entry to the permissions list.
Nit: if you end a comment with a full stop (.) then you should start with a capital letter.

>+      // We do not hava a principal yet so we use only the origin as identification.
Nit: typo, should be have rather than hava.

>+    // we are not done yet
>+    // not sure if this is ever called
I guess it does if it is not a type as listed in the first switch.

>   reactToChange: function permissions_reactToChange(aSubject, aData) {
>+
>+    // aData: added, changed, deleted, cleared
>+    // asubject: the subject which is the permission to be changed.
Nit: aSubject rather than asubject (case of the letter "s").

>+    // Does change affects possibly loaded Preferences pane?
Nit: affect rather than affects

>+      for (let lChild of this.list.children) {
What is wrong with using child rather than lChild?

>+        // check type and host (origin) first.
Nit: if you end a comment with a full stop (.) then you should start with a capital letter.

>+          // check if them permission list contains the principal.
Nit: if you end a comment with a full stop (.) then you should start with a capital letter.

>+          // If not add it to the permissions list
Nit: adding rather than add.
>+          // this might be the case for newly created items
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+  // This function is a called when you check that all permissions for the given domain should be
>+  // deleted (forget)
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+  addButtonClick: function PermissionsPwd_addButtonClick() {
>+    gDataman.debugMsg("Add password permissions button clicked!");
>+    // this.addSelBox, this.addHost, this.addType, this.addButton
What is this comment about?

>+      for (let lChild of this.list.children) {
Why use lChild rather than child?

>+      for (let lSignon of this.allSignons) {
Why not just use signon rather than lSignon?

>+    for (let lGroup of groups) {
group instead of lGroup?

>+    for (let lStorage of this.storages) {
storage instead of lStorage?

>   reactToChange: function storage_reactToChange(aSubject, aData) {
>     // aData: null (sessionStorage, localStorage) + nsIDOMStorageEvent in aSubject
>     //        --- for appCache and indexedDB, no change notifications are known!
>     //        --- because of that, we don't do anything here and instead use
>     //            reloadList periodically
>+    let type = aData;
Why not just alter the debugMsg line to use aData?

>+++ b/suite/common/dataman/dataman.xml

>+        var permPwdLabel = this.type;
>+        try {
>+          permPwdLabel = gDataman.bundle.getString("permpwd." + this.type + ".label");
>+        }
>+        catch (e) {
>+        }
As before need to make sure that the exception doesn't change the variable permPwdLabel.

>+++ b/suite/common/permissions/cookieViewer.js

>+  for (let lProp of props)
Why not prop instead of lProp?


>+    for (let i of deletedCookies) {
>+      allCookies.splice(allCookies.indexOf(i), 1);
Why not cookie instead of i?

>+  for (let delCookie of deletedCookies) {
Why not cookie instead of delCookie?

>+    // We are only interested in cookie permissions in this code
Nit: if you start a comment with a capital letter it should end with a full stop (.).

>+      // it is currently possible to add a cookie permission for about:xxx and other internal pages.
Nit: if you end a comment with a full stop (.) then you should start with a capital letter.

>+      // They are probably invalid and will be ignored for now
>+      // test if the permission has a host
Nit: if you start a comment with a capital letter it should end with a full stop (.).


>+   // only allow a few schemes here
Nit: indent incorrect.
>+  // others like file:// would produce an invalid entry in the database

>+  for (let p of deletedPermissions)
Either perm or permission instead of p?

>+++ b/suite/common/permissions/cookieViewer.xul

>               <treecol id="siteCol" label="&treehead.sitename.label;" flex="5"
>-                           onclick="PermissionColumnSort('rawHost', true);" persist="width"
>+                           onclick="PermissionColumnSort('host', true);" persist="width"
>                            sortDirection="ascending"/>
Tend to have one attribute per line and the indentation is incorrect, onclick should line up with id.

>               <splitter class="tree-splitter"/>
>+              <treecol id="siteCol2" label="&treehead.scheme.label;" flex="5"
>+                           onclick="PermissionColumnSort('scheme', true);" persist="width"/>
Tend to have one attribute per line and the indentation is incorrect, onclick should line up with id.
>+                <splitter class="tree-splitter"/>
Indentation is incorrect.

f+ for the moment.
Attachment #8714105 - Flags: superreview?(iann_bugzilla) → feedback+

Comment 103

3 years ago
>>+  checkValidPermforDataman: function dataman_checkValidPermforDataman(aPermission) {
> Can this not just be called checkValidPermission?

Also, our current best practices is not to decorate object methods. e.g.:
  checkValidPermission: function(aPermission) {...},

(Unlike the old debugger, the current V2 debugger is smart enough to extract the method name from the object)
ES6 syntax is even more compact (SpiderMonkey supports this):
  checkValidPermission(aPermission) {...},

Comment 104

3 years ago
(In reply to Philip Chee from comment #103)
> >>+  checkValidPermforDataman: function dataman_checkValidPermforDataman(aPermission) {
> > Can this not just be called checkValidPermission?
> 
> Also, our current best practices is not to decorate object methods.

I do not think that coding style of a file should be changed with a bugfix, and the coding style of the file should be consistent. That said, I handed off that code, it's not my decision, just an observation.
Assignee

Comment 105

3 years ago
I do not have much time till next weekend. Will see that I fix the formal errors then and check the points made by Ian. I would rather not doing anything more until checked in. I am quite sure you can always find something else but the Data manager is already broken too long :)

I tried to blend the patches in and didn't think much about different or new coding styles so I agree with Robert.
Assignee

Comment 106

3 years ago
>> This is a very bif patch, is there no way of splitting into the permissionPwd part and then everything else?

Ian,

updated path but I didn't set review yet. See below.

I ripped out the password part while fixing the permissions and then put it back without realizing how big the patch really got. I would rather not split it now. Its tested and I might break something else again. What I can do easily is separate the cookieviewer and dataman parts.

>+++ b/suite/common/dataman/dataman.js

>> >+      for (let rHost of rejectHosts)
>> >+        gDomains.addDomainOrFlag(rHost, "hasPermissionsPwd");
>> Not sure if host is already in use, but if not why not just use that instead of rHost?

I just like to have unique names for later lookup or if something goes wrong. Just a personal preference. Helps me to fix things faster and avoids mixups of you use the same generic variable in the code where it shouldn't be used. 

>> Is this a host or a signon?

Its a signon. Name changed.

>> Is this a storage rather than a host?

storage. Name changed.

>> Should this be field instead of i?

Right. More consistent this way.

>> >+//                              gLocSvc.idn.convertToDisplayIDN(rawHost, {}));
>> Delete rather than comment out?

Oki. Thought this might need an additional fix later when someone reports an issue with an IDN domain so I left it in as a marker.

>> >+        let typeDesc = permType;
>> >         try {
>> >+          typeDesc = gDataman.bundle.getString("perm." + permType + ".label");
>> >         }
>> >         catch (e) {
>> >         }
>> If catch is triggered what is typeDesc left as?

Old code. Stumbled over it too. typeDesc would be the raw untranslated permission type if the assignment fails. I think this should work ok. If it's unsafe I could set it in the catch.

>> >+          gDomains.selectedDomain.title == "*" ? "" : ("http://www." + gDomains.selectedDomain.title);
>> Is it always going to be correct being http and www?

No. Just a default and a hint what needs to be entered. The user can overtype it with https, enter only a domain witha sheme or whatever he/she wants. In most cases I think this will be right.

>+        nOrigin = new URL(this.addHost.value).origin;
...
>> Does this need to be in the catch, doesn't it get picked up in the if statement below?

Yes. If you enter complete nonsense this will throw. The second one will just pick up the tricky cases like fkffjj://hubbub.com. They might be put together in one try block but then you can distinguish the errors.

>> >+    // we are not done yet
>> >+    // not sure if this is ever called
>> I guess it does if it is not a type as listed in the first switch.

Thought so too and canged it already in my private tree.


>> >+      for (let lChild of this.list.children) {
>> What is wrong with using child rather than lChild?

Same as host above. Just personal preference. I try to avoid too generic names.

>> >+      for (let lChild of this.list.children) {
>> Why use lChild rather than child?

See above
>> >+      for (let lSignon of this.allSignons) {
>> Why not just use signon rather than lSignon?
See above

>> >+    for (let lGroup of groups) {
>> group instead of lGroup?
See above
>+    for (let lStorage of this.storages) {
storage instead of lStorage?
See above

>+++ b/suite/common/dataman/dataman.xml

>> As before need to make sure that the exception doesn't change the variable permPwdLabel.
Try/Catch removed. We ware dealing only with passwords here. 

>+++ b/suite/common/permissions/cookieViewer.js

>> >+  for (let lProp of props)
>> Why not prop instead of lProp?
see above

>> Why not cookie instead of i?
Oki

>> Why not cookie instead of delCookie?
See above

>+++ b/suite/common/permissions/cookieViewer.xul

>> Tend to have one attribute per line and the indentation is incorrect, onclick should line up with id.

Really old code. Didn't do much/anything in the first place to it. Reformatted it a lot. Check if ok now.


Aside from the variable renames I changed everything that was suggested I think. If you want me to change the variable names too let me know and I will do it tomorrow. Would still dislike so many generic names but do not want to anger the natives:)

FRG (thank god its Friday!)
Attachment #8714105 - Attachment is obsolete: true
Attachment #8714105 - Flags: review?(neil)
Attachment #8724194 - Flags: feedback?(iann_bugzilla)

Comment 107

3 years ago
My 2¢: I too would prefer child, signon, prop etc.

> I just like to have unique names for later lookup or if something goes 
> wrong. Just a personal preference. Helps me to fix things faster and avoids 
> mixups of you use the same generic variable in the code where it shouldn't 
> be used. 

Well you have two instances of:
  let lChild of this.list.children
So that defeats your purpose.
Assignee

Comment 108

3 years ago
>> So that defeats your purpose.

Yupp but they are in different functions. I won't even try to make them unique in the whole source code. Just dislike and try to avoid bland variable names like host, name, tree or url. In my good old z/OS days some names were sometimes also reserverd compiler variables and you couldn't even use them. It's just a habit that helps me to understand what's going on in the code faster. If you want me to change them just let me know. 

FRG

Comment 109

3 years ago
Comment on attachment 8724194 [details] [diff] [review]
cc-1188348-permission-part2-V7.patch

(In reply to Frank-Rainer Grahl from comment #106)
> Created attachment 8724194 [details] [diff] [review]
> cc-1188348-permission-part2-V7.patch
> 
> >> >+//                              gLocSvc.idn.convertToDisplayIDN(rawHost, {}));
> >> Delete rather than comment out?
> 
> Oki. Thought this might need an additional fix later when someone reports an
> issue with an IDN domain so I left it in as a marker.
If you want to leave a marker, then add a comment about IDN domain.

> >+++ b/suite/common/permissions/cookieViewer.xul
> 
> >> Tend to have one attribute per line and the indentation is incorrect, onclick should line up with id.
> 
> Really old code. Didn't do much/anything in the first place to it.
> Reformatted it a lot. Check if ok now.
I know reformatting of the whole file improves the readability but messes up the blame so only reformat the areas you are changing.
Attachment #8724194 - Flags: feedback?(iann_bugzilla) → feedback+
Assignee

Comment 110

3 years ago
Patch with formatting corrected in both xuls for changed parts and idn comment removed
Attachment #8724194 - Attachment is obsolete: true
Attachment #8725273 - Flags: review?(iann_bugzilla)
Assignee

Comment 111

3 years ago
l10n string additions only. Taken from version 8 patch for reference.

Comment 112

3 years ago
(In reply to Frank-Rainer Grahl from comment #110)
> Created attachment 8725273 [details] [diff] [review]
> cc-1188348-permission-part2-V8.patch
> 
> Patch with formatting corrected in both xuls for changed parts and idn
> comment removed

I thought you would add a comment about IDN rather than just removing the comment line completely (gLocSvc.idn.convertToDisplayIDN(rawHost, {}));)
Flags: needinfo?(frgrahl)
Assignee

Comment 113

3 years ago
I had the line already removed in V7 and looked over the patch again. I think its need to be done in a few places and in the old cookie manager too so I didn't add it there. Didn't have much time Tuesday and left it just out. I will look at it in a followup patch. Will see about the notification box and a few stray cases + anything that might come up when more people test it

FRG
Flags: needinfo?(frgrahl)

Comment 114

3 years ago
1. Your changes to dataman.xul introduces new trailing whitespace.
2. Do your changes to cookie viewer include the fix for Bug 1251368 (SeaMonkey cookie code needs to be updated to take into account mozilla-central Bug 1245184)?
Flags: needinfo?(frgrahl)
Assignee

Comment 115

3 years ago
I see. Goofed with the whitespaces. 

I didn't change the remove code. I am not sure how it's possible to delete cookies for another user. I thought this was all profile specific but it looks like remove needs to be changed. I can look at it next when I finish with the safebrowsing code.
Attachment #8725273 - Attachment is obsolete: true
Attachment #8725273 - Flags: review?(iann_bugzilla)
Flags: needinfo?(frgrahl)
Attachment #8727196 - Flags: review?(iann_bugzilla)
Assignee

Updated

3 years ago
Blocks: 1251368

Updated

3 years ago
See Also: → 1257311

Updated

3 years ago
Attachment #8727196 - Flags: review?(iann_bugzilla) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 116

3 years ago
http://hg.mozilla.org/comm-central/rev/742f46dc17a4
Keywords: checkin-needed
Whiteboard: [workaround see comments #30 and #92] → [workaround see comments #30 and #92][leave open]
Target Milestone: --- → seamonkey2.45
Assignee

Comment 117

3 years ago
Hi,

can we close the bug or can it be pushed to c-a despite the l10n changes?
Flags: needinfo?(philip.chee)

Comment 118

3 years ago
(In reply to Frank-Rainer Grahl from comment #117)
> Hi,
> 
> can we close the bug or can it be pushed to c-a despite the l10n changes?
IanN should know. Let's ask IanN
Flags: needinfo?(philip.chee) → needinfo?(iann_bugzilla)

Comment 119

3 years ago
Is there anyway of using existing strings to produce a l10n safe patch?
If it was at the very start of the cycle then a late l10n notice could be posted to m.d.l10n
Flags: needinfo?(iann_bugzilla) → needinfo?(frgrahl)
Assignee

Comment 120

3 years ago
>> Is there anyway of using existing strings to produce a l10n safe patch?

If I replace the errormessages with something generic from somewhere else it might be possible. Biggest problem are the scheme column names but this might be also found somewhere else. 

Just found a typo in cookieViewer.proberties: errorAddPermisison. I am trying to fix it today so that it can go with c-c into c-a tomorrow.
Flags: needinfo?(frgrahl)

Updated

3 years ago
Depends on: 1267057
Assignee

Updated

3 years ago
Blocks: 1269195
Assignee

Comment 121

3 years ago
I worked on another Data Manager fix today and in the process checked again if this one could be ported back to 2.43 and 2.44. I now do not think this is not possible without some string changes or a complete new alternate patch only for c-b and c-r. 

I am closing this one. The do it yourself fix should do until 2.45 arrives. If someone thinks it still can be done in another way please reopen.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Whiteboard: [workaround see comments #30 and #92][leave open] → [workaround see comments #30 and #92]
Assignee

Updated

3 years ago
Duplicate of this bug: 1228737

Updated

3 years ago
See Also: → 1288775

Comment 123

2 years ago
This doesn't seem to be fixed for me on Windows 10 or Linux, both SeaMonkey 2.46 
Only the cookie tab works, others are greyed out.

Console shows

>Error: TypeError: nextPermission.host is undefined 
>Source File: chrome://dataman/content/dataman.js  Line: 312

Confirm passwords exist, as they still fill in, and are visible from chrome://passwordmgr/content/passwordManager.xul

I can provide more info as needed, or file a separate bug if preferred.

Comment 124

2 years ago
(In reply to Kelly Clowers from comment #123)
> This doesn't seem to be fixed for me on Windows 10 or Linux, both SeaMonkey
> 2.46 
> Only the cookie tab works, others are greyed out.
> 
> Console shows
> 
> >Error: TypeError: nextPermission.host is undefined 
> >Source File: chrome://dataman/content/dataman.js  Line: 312
> 
> Confirm passwords exist, as they still fill in, and are visible from
> chrome://passwordmgr/content/passwordManager.xul
> 
> I can provide more info as needed, or file a separate bug if preferred.

Hi, it does look like you have a version of the dataman add-on installed into you profile as that line was one of the lines removed by the patch from this bug, which is more of a support issue than a bug.
One way to test is to create a new profile and see if the problem is still there.
Assignee

Comment 125

2 years ago
Jupp. This is the old code.

Line 312 in 2.46 is:
 let rejectHosts = Services.logins.getAllDisabledHosts();
Assignee

Comment 126

2 years ago
Dataman is at:

chrome://communicator/content/dataman/dataman.xul

You can try to enter this url in the location bar to verify that you have some old cruft in your profile.

FRG

Comment 127

2 years ago
Thank you! Yes, I have used this profile for years, and used dataman from almost its beginning. Disabled it and everything works!
You need to log in before you can comment on or make changes to this bug.