1045989 - Data Manager uses nonexistent API nsIDOMStorageManager::getUsage

Status: NEW

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Peter B. Shalimoff]

Attachment: dataman.js.patch

Data Manager uses nonexistent API nsIDOMStorageManager::getUsage - see dataman.js, line 2344 (http://mxr.mozilla.org/comm-central/source/suite/common/dataman/dataman.js#2344). So here's a patch.

Comment 1

[Peter B. Shalimoff]

Attachment: dataman.js.patch

Sorry, that was the wrong patch, here's the correct one.

Comment 2

[Philip Chee]

Comment on attachment 8464464 [details] [diff] [review]
dataman.js.patch

(In reply to Peter B. Shalimoff from comment #0)
> Created attachment 8464463 [details] [diff] [review]
> dataman.js.patch
> 
> Data Manager uses nonexistent API nsIDOMStorageManager::getUsage - see
> dataman.js, line 2344
> (http://mxr.mozilla.org/comm-central/source/suite/common/dataman/dataman.
> js#2344). So here's a patch.

Hi! thanks for the patch. There are probably some issues with following Mozilla coding style. In the mean time, I'm asking Robert Kaiser - the author of the Data Manager component - for feedback

Comment 3

[[e-mail]]

Comment on attachment 8464464 [details] [diff] [review]
dataman.js.patch

OK, so from a style POV, there are a lot of unnecessary or incorrect style changes, so it's easier for me to list the correct changes:

>-              connection.createStatement("SELECT scope, key FROM webappsstore2");
>+          statement = connection.createStatement("select scope, sum(length(key) + length(value)) from webappsstore2 group by scope");
Only the change to the SQL string parameter to createStatement with the SQL keywords restored to upper case.

>-                               key: statement.getString(1)});
>+                               size: statement.getInt32(1)});

>-            this.storages[j].keys.push(domstorelist[i].key);

>-                              size: gLocSvc.domstoremgr.getUsage(rawHost),
>-                              origHost: origHost,
>-                              keys: [domstorelist[i].key]});
>+                              size: domstorelist[i].size,
>+                              origHost: origHost});
This change is OK, although you could have potentially exchanged the size and origHost lines.

Apart from that, thanks for the patch!

Comment 4

[[e-mail]]

(Looks like getUsage was removed by bug 600307 for SeaMonkey 20 so this has been broken for some time. Oops.)

Comment 5

[Robert Kaiser]

Comment on attachment 8464464 [details] [diff] [review]
dataman.js.patch

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

I can't give a + because I don't understand what it actually does, though the approach might be OK, so no - either.

That said, it may take a bit (though hopefully not quite as long when I'm not on vacation - was in the middle of 3 weeks of vacation when this f? came in) but I'm still happy to actually do reviews for Data Manager as I think I still own it. ;-)

::: D:/TEMP/1/old/chrome/comm/content/communicator/dataman/dataman.js
@@ +2307,4 @@
>          }
>        } finally {
> +        if (statement) { statement.finalize(); }
> +        if (connection) { connection.close(); }

Why all those other changes? Why do we need that comment on the "owner" column? What does that have to do with what we are doing here? Wouldn't a comment on the magical sum() be a better idea instead? I have no idea what that one actually does.

@@ +2343,5 @@
>            this.storages.push({host: host,
>                                rawHost: rawHost,
>                                type: type,
> +                              size: domstorelist[i].size,
> +                              origHost: origHost});

I don't remember off-hand because it's been such a long time since I worked on this, but could you explain why you are removing the keys in both of those cases?

Comment 6

[Peter B. Shalimoff]

(In reply to Robert Kaiser (:[e-mail]) from comment #5)
> I can't give a + because I don't understand what it actually does, though
> the approach might be OK, so no - either.

You can always give it a - and fix the bug yourself. It is still valid even without the patch, and I don't really care which way it's fixed. Maybe it would be better to simply drop the Size column, as it's rather useless: noobs don't care (or even know) about such thing as local storage, let alone its size, and those who've been on the Internet for at most an hour already know that in 99.(9)% cases the reason a web site stores something on your computer is to track you, the only correct answer to which is "go track yourself" followed by a complete wipe of whatever **** they might have stored, regardless of its size.

> ::: D:/TEMP/1/old/chrome/comm/content/communicator/dataman/dataman.js
> @@ +2307,4 @@
> >          }
> >        } finally {
> > +        if (statement) { statement.finalize(); }
> > +        if (connection) { connection.close(); }
> 
> Why all those other changes? Why do we need that comment on the "owner"
> column? What does that have to do with what we are doing here?

You're right, these changes and the comment are unnecessary.

As for sum(), I didn't comment on it because it's basics, and commenting on it is like commenting a array.forEach() call, redundant. It calculates the size each scope (host) has, which is the sum of the sizes (lengths) of all its keys and values. As to how aggregate functions (sum() is one of them) work, google it? Someone at Oracle wrote a nice article on it [1], and someone even made some pictures [2][3].

> @@ +2343,5 @@
> >            this.storages.push({host: host,
> >                                rawHost: rawHost,
> >                                type: type,
> > +                              size: domstorelist[i].size,
> > +                              origHost: origHost});
> 
> I don't remember off-hand because it's been such a long time since I worked
> on this, but could you explain why you are removing the keys in both of
> those cases?

Because they are not used anywhere.


[1] http://www.oracle.com/technetwork/issue-archive/2013/13-jan/o13sql-1886636.html
[2] http://www.tutorialscollection.com/sql-group-by-how-to-use-group-by-in-sql-with-sum-count-etc/
[3] http://www.tutorialscollection.com/sql-demo/sql-demo.php?ex=14.0_1

Comment 7

[Tony Mechelynck [:tonymec]]

UA "Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0 SeaMonkey/2.40a1" ID:20150920003001 c-c:998a6fac4571fc297af324ad8d8a92e9ea3faac1 m-c:ccd6b5f5e544c1d708849144943a776941bd3794

The Data Manager is severely broken, I can't access anything except cookies (for specific hosts) and form data (for the * host). The * host also has a "Permissions" tab which is not greyed-out and where I seem to be able to add permissions, but there's no way I can check that it has indeed been added.

I suspect this bug is the cause of the problem.

Comment 8

[Tony Mechelynck [:tonymec]]

(In reply to Tony Mechelynck [:tonymec] from comment #7)
> UA "Mozilla/5.0 (X11; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
> SeaMonkey/2.40a1" ID:20150920003001
> c-c:998a6fac4571fc297af324ad8d8a92e9ea3faac1
> m-c:ccd6b5f5e544c1d708849144943a776941bd3794
> 
> The Data Manager is severely broken, I can't access anything except cookies
> (for specific hosts) and form data (for the * host). The * host also has a
> "Permissions" tab which is not greyed-out and where I seem to be able to add
> permissions, but there's no way I can check that it has indeed been added.

By checking with a site where adding an add-on is blocked, I see that the new permission does indeed _not_ take effect: after setting the Install Software perm for that site to "Allow" I still get the doorhanger telling me that it has been blocked.

> 
> I suspect this bug is the cause of the problem.

Comment 9

[Robert Kaiser]

Please file a different bug on the general brokenness, this bug is a very specific thing that only breaks the Web Storage panel.

Comment 10

[Tony Mechelynck [:tonymec]]

(In reply to Robert Kaiser (:[e-mail]) from comment #9)
> Please file a different bug on the general brokenness, this bug is a very
> specific thing that only breaks the Web Storage panel.

Reported bug 1208700 with you and Neil as Cc. You should have got the bugmail by now.

Comment 11

[Frank-Rainer Grahl (:frg)]

I stumbled over this yesterday when I found another error in Data Manager. 

The instanceof in line 2570 is invalid:

>> if (aSubject instanceof Components.interfaces.nsIDOMStorageEvent) {

Picked some code from the Firefox tests and fixed this but then encountered the getusage error.

I just set size to 0 in the patch for Bug 1188348. As you said nobody seems to use it.

Comment 12

[Tony Mechelynck [:tonymec]]

Are all recent builds still "affected"? Then every version lower than 2.39 may be set to "wontfix" since 2.39 is the current release and nothing lower than that will ever be fixed in the future.

Comment 13

[Frank-Rainer Grahl (:frg)]

Yes. All up to comm-central are affected by this bug and the remaining issues in Bug 1188348.

Comment 14

[Tony Mechelynck [:tonymec]]

OK. I suppose one "wontfix" in the Tracking Flags (the latest one known for certain) is enough.