594478 - IndexedDB needs to be hooked up with "Forget about this site"

Status: VERIFIED FIXED

(Toolkit :: Data Sanitization, defect)

Description

[Shawn Wilsher :sdwilsh]

I don't think we can ship IndexedDB without this since it breaks one of our privacy features.

Comment 1

[(no longer active)]

Shawn, can you take this?  I don't really know anything about indexed DB internals, or what should be done here.

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> Shawn, can you take this?  I don't really know anything about indexed DB
> internals, or what should be done here.
I don't have any cycles for quite a while right now (places blocker count is kinda large in terms of workload).  bent can probably tell you anything you'd need to know (or I could; doesn't really matter).

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

You probably only need to do this:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/pageinfo/permissions.js#216

Comment 4

[(no longer active)]

Does Indexed DB use a disk backed storage object?  If so, that should probably be either disabled in PB mode, or we should switch to a separate memory based DB, right?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

IndexedDB is entirely disabled in PB mode already.

Comment 6

[(no longer active)]

(In reply to comment #5)
> IndexedDB is entirely disabled in PB mode already.

Oh, I didn't know that.  We do have tests for that, right?

And in that case, what did you mean in comment 3?  And what's the purpose of this bug?

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Oh, I didn't know that.  We do have tests for that, right?
Probably not...

> And in that case, what did you mean in comment 3?  And what's the purpose of
> this bug?
We need to make right clicking and selecting "forget about this site" work with indexedDb.  Right now, it doesn't clear it.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #7)
> (In reply to comment #6)
> > Oh, I didn't know that.  We do have tests for that, right?
> Probably not...

I actually have one written but it used to leak (unrelated to IndexedDB - if you commented out everything but enter/exit PB then it leaked the world).

I'll dust it off at some point and see if it still leaks.

Comment 9

[(no longer active)]

(In reply to comment #7)
> (In reply to comment #6)
> > Oh, I didn't know that.  We do have tests for that, right?
> Probably not...

If you tell me how they're "disabled", I'll file a bug and write a test for that.

> > And in that case, what did you mean in comment 3?  And what's the purpose of
> > this bug?
> We need to make right clicking and selecting "forget about this site" work with
> indexedDb.  Right now, it doesn't clear it.

Well, of course.  I somehow forgot what this bug is all about, and thought that it's about Indexed DB/PB interaction!  Embarrassing!

Comment 10

[(no longer active)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Oh, I didn't know that.  We do have tests for that, right?
> > Probably not...
> 
> I actually have one written but it used to leak (unrelated to IndexedDB - if
> you commented out everything but enter/exit PB then it leaked the world).
> 
> I'll dust it off at some point and see if it still leaks.

Land it away!  (Bug 611089 was the reason, I bet)

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #10)
> Land it away!  (Bug 611089 was the reason, I bet)

Nice. I'll retry it.

Comment 12

[(no longer active)]

Attachment: WIP

I wrote this patch blindly.  I don't know of any indexed DB samples on the web to test this against, so I tried to write a test case for it.  The test case seems to be timing out for some reason, which I couldn't figure out.

Note the alerts I injected in the test.  "1" and "2" get alerted, but "x" never does.  So seems like attempting to setVersion on db2 is screwing something up, but I don't know why.  Also, the error handler is not called either, so the test just sits there after alerting "2" and then times out.

Shawn, can you think of what I'm doing wrong?  Also, can you please point me to a (mini-)web app which uses indexed DB so that I can test this patch manually?

Thanks!

Comment 13

[(no longer active)]

sdwilsh: ping?

Comment 14

[Shawn Wilsher :sdwilsh]

(In reply to comment #12)
> Shawn, can you think of what I'm doing wrong?  Also, can you please point me to
> a (mini-)web app which uses indexed DB so that I can test this patch manually?
There's isn't one out there really.  You could look at our tests for it I guess.

Comment 15

[Shawn Wilsher :sdwilsh]

Your code seems to be assuming that open takes the url of the db you are going to open, which isn't right.  It's the name:
http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#requests

I don't think that's the issue, but maybe?

Comment 16

[(no longer active)]

(In reply to comment #15)
> Your code seems to be assuming that open takes the url of the db you are going
> to open, which isn't right.  It's the name:
> http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#requests
> 
> I don't think that's the issue, but maybe?

So, in order to test this, I need to at least open indexed DB's for two domains, so that I can make sure that the entries in one of them gets deleted, and the other is not touched.  How can I do that?

Comment 17

[(no longer active)]

(And please note that the param being the name doesn't still explain why the test is failing, does it?)

Comment 18

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> So, in order to test this, I need to at least open indexed DB's for two
> domains, so that I can make sure that the entries in one of them gets deleted,
> and the other is not touched.  How can I do that?
mochitest has a few domains it can use (example.com, mochi.test) and you'd just have to load the test pages in a subframe or something.

(In reply to comment #17)
> (And please note that the param being the name doesn't still explain why the
> test is failing, does it?)
Your error callback never happens?

Comment 19

[(no longer active)]

(In reply to comment #18)
> (In reply to comment #16)
> > So, in order to test this, I need to at least open indexed DB's for two
> > domains, so that I can make sure that the entries in one of them gets deleted,
> > and the other is not touched.  How can I do that?
> mochitest has a few domains it can use (example.com, mochi.test) and you'd just
> have to load the test pages in a subframe or something.

I'll give that a shot, thanks!

> (In reply to comment #17)
> > (And please note that the param being the name doesn't still explain why the
> > test is failing, does it?)
> Your error callback never happens?

No, nothing is called, and the test just times out :(

Comment 20

[(no longer active)]

Attachment: WIP 2

So I tried to split the test across domains, and I got the exact same result this time too.  Shawn, I'm basically out of ideas at this point.

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 497391 [details] [diff] [review]
WIP 2

bent can probably look at this sooner than I can

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: New tests, a few changes

Jonas, can you look over the tiny non-test change?

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 502068 [details] [diff] [review]
New tests, a few changes

r=me on the IndexedDatabaseManager.cpp changes.

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 502068 [details] [diff] [review]
New tests, a few changes

And r=me on the private browsing changes.

Comment 25

[(no longer active)]

The test times out on Linux/Linux64 optimized builds:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1294469817.1294471001.32204.gz&fulltext=1#err4

Ben, do you have any idea why?

Also, the test looked kind of strange to me.  You don't actually write any data into the databases.  What does setting the DB version really mean?  Is Forget About This Site expected to clear the DB version as well?

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, fixed test

No code changes, only test changes.

I found the bug, I think, this one passes all try servers. Async tests blow.

Anyway, setting the version of a database is a significant event. Forgetting the website should reset the database version.

Comment 27

[(no longer active)]

OK then!

Comment 28

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/14ccd69459d6

Comment 29

[u279076]

What can be done to verify this bug is fixed?

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We've got an automated test that checks here.