Closed Bug 594478 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Data Sanitization, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: sdwilsh, Assigned: ehsan)

References

()

Details

(Whiteboard: [softblocker])

Attachments

(1 file, 3 obsolete files)

I don't think we can ship IndexedDB without this since it breaks one of our privacy features.
blocking2.0: --- → final+
Shawn, can you take this?  I don't really know anything about indexed DB internals, or what should be done here.
(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).
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?
IndexedDB is entirely disabled in PB mode already.
(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?
(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.
(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.
(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!
(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)
Assignee: nobody → ehsan
(In reply to comment #10)
> Land it away!  (Bug 611089 was the reason, I bet)

Nice. I'll retry it.
Attached patch WIP (obsolete) — Splinter Review
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!
Attachment #493464 - Flags: feedback?(sdwilsh)
Whiteboard: [has WIP][needs feedback sdwilsh]
sdwilsh: ping?
(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.
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?
(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?
(And please note that the param being the name doesn't still explain why the test is failing, does it?)
(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?
(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 :(
Attached patch WIP 2 (obsolete) — Splinter Review
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.
Attachment #493464 - Attachment is obsolete: true
Attachment #497391 - Flags: review?(sdwilsh)
Attachment #493464 - Flags: feedback?(sdwilsh)
Comment on attachment 497391 [details] [diff] [review]
WIP 2

bent can probably look at this sooner than I can
Attachment #497391 - Flags: review?(sdwilsh) → review?(bent.mozilla)
Whiteboard: [has WIP][needs feedback sdwilsh] → [has WIP][needs feedback sdwilsh][indexeddb]
Attached patch New tests, a few changes (obsolete) — Splinter Review
Jonas, can you look over the tiny non-test change?
Attachment #497391 - Attachment is obsolete: true
Attachment #502068 - Flags: review?(jonas)
Attachment #497391 - Flags: review?(bent.mozilla)
Comment on attachment 502068 [details] [diff] [review]
New tests, a few changes

r=me on the IndexedDatabaseManager.cpp changes.
Attachment #502068 - Flags: review?(jonas) → review+
Comment on attachment 502068 [details] [diff] [review]
New tests, a few changes

And r=me on the private browsing changes.
Attachment #502068 - Flags: review+
Whiteboard: [has WIP][needs feedback sdwilsh][indexeddb] → [needs landing]
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?
Whiteboard: [needs landing] → [needs new patch]
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.
Attachment #502068 - Attachment is obsolete: true
Attachment #502490 - Flags: review+
OK then!
Whiteboard: [needs new patch] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/14ccd69459d6
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → Firefox 4.0b10
Whiteboard: [softblocker]
What can be done to verify this bug is fixed?
We've got an automated test that checks here.
Status: RESOLVED → VERIFIED
Component: Private Browsing → Forget About Site
Product: Firefox → Toolkit
Target Milestone: Firefox 4.0b10 → ---
You need to log in before you can comment on or make changes to this bug.