After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 741447 - Clear personal data should allow clearing all stored data
Clear personal data should allow clearing all stored data
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
git master
Other All
: Normal normal
: ---
Assigned To: Carlos Garcia Campos
Epiphany Maintainers
Depends on: 737808
Blocks:
 
 
Reported: 2014-12-12 15:38 UTC by Michael Catanzaro
Modified: 2017-02-01 06:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Clear temporary files should clear HTML5 local storage (2.94 KB, patch)
2014-12-12 15:38 UTC, Michael Catanzaro
none Details | Review
Rework personal data dialog using new WebKit API (33.59 KB, patch)
2017-01-23 18:14 UTC, Carlos Garcia Campos
committed Details | Review

Description Michael Catanzaro 2014-12-12 15:38:01 UTC
HTML5 local storage is left on disk after clearing temporary files.

A couple thoughts:

* It's possible that we might want to have separate UI for local storage, on the grounds that web sites may not consider local storage to be temporary. (The disadvantage would be possibly unnecessary complexity.)
* We might want this functionality WebKitWebContext instead.
Comment 1 Michael Catanzaro 2014-12-12 15:38:03 UTC
Created attachment 292616 [details] [review]
Clear temporary files should clear HTML5 local storage

Clear temporary files currently clears the cache and favicon database,
but leaves all local storage. We should probably delete that too.
Comment 2 Carlos Garcia Campos 2015-02-03 12:02:30 UTC
So this depends on bug #737808, that has a patch not ready to land. We should try to fix these bugs before the freeze.
Comment 3 Michael Catanzaro 2015-02-08 15:45:46 UTC
We have more problems in this area than just localstorage. In particular, I'm trying to figure out why I have ~/.local/share/webkit/databases (instead of ~/.local/share/webkitgtk/databases) and ~/.local/share/webkit/icondatabase (in addition to ~/.cache/epiphany/icondatabase). I guess the values we specify in WebProcessPoolGtk.cpp are not all honored.

We might need to reconsider the API added in https://bugs.webkit.org/show_bug.cgi?id=138828. I was thinking that a website-data-directory property, specifying a directory under which directories for localstorage and databases (and eventually mediakeys) would be created, would work better.
Comment 4 Carlos Garcia Campos 2015-02-09 08:07:15 UTC
(In reply to comment #3)
> We have more problems in this area than just localstorage. In particular, I'm
> trying to figure out why I have ~/.local/share/webkit/databases (instead of
> ~/.local/share/webkitgtk/databases) and ~/.local/share/webkit/icondatabase (in
> addition to ~/.cache/epiphany/icondatabase). I guess the values we specify in
> WebProcessPoolGtk.cpp are not all honored.

Because that have changed several times and very inconsistenly.

> We might need to reconsider the API added in
> https://bugs.webkit.org/show_bug.cgi?id=138828. I was thinking that a
> website-data-directory property, specifying a directory under which directories
> for localstorage and databases (and eventually mediakeys) would be created,
> would work better.

I think it's important to have a separate directory for every feature, you can set all to the same dir if you want. In the case of favicon database, if you don't set a directory, the feature is not enabled. In other cases like IndexedDB, subdirectories are created with different schemas, so I would rather keep every kind of database in its own directory.
Comment 5 Carlos Garcia Campos 2017-01-23 18:14:14 UTC
Created attachment 344058 [details] [review]
Rework personal data dialog using new WebKit API

This uses the new WebKit API, we need a new release to depend on. I'm sure the UI needs some tweaks, but the functionality if there.
Comment 6 Michael Catanzaro 2017-01-24 04:54:35 UTC
Review of attachment 344058 [details] [review]:

Really nice, it looks great! I prefer you wait until the next WebKit release to commit it.

One bad thing I notice is the type to search doesn't work, the input gets captured by the tree view instead of by your search box. So please fix that; anywhere the user can see a search button, typing a normal character should activate the button and begin a search.

Another bad thing is that I still have a bunch of application cache files left over (including a 10 MB one) after deleting all application cache from the beginning of time. But I guess that's what you fixed in https://trac.webkit.org/changeset/211035, right?

The biggest problem is that you can only clear data for particular sites if selecting the beginning of time option. I don't care about the restriction, but I do about how it's presented to users: there's no way to know that you need to select "from the beginning of time" in order to clear data for particular sites. So we need something better here. One easy solution would be to change the default state of the combo box to "from the beginning of time".

I'd make one incidental change to the prefs dialog itself: change the "Clear personal data..." button to "Manager personal data..." so that users don't fear that pressing the button is destructive.

If you want bonus points, store in GSettings which of the top-level checkboxes are checked: bug #770755.

::: src/clear-data-dialog.c
@@ +147,3 @@
+
+  treestore = GTK_TREE_STORE (dialog->treestore);
+  for (guint i = 0; i < G_N_ELEMENTS(data_entries); i++) {

G_N_ELEMENTS (data_entries)

@@ +178,3 @@
+  }
+
+  g_list_free_full (data_list, (GDestroyNotify)webkit_website_data_unref);

Is the cast really needed?

@@ -127,2 +387,9 @@
-  gtk_widget_set_sensitive (dialog->clear_button,
-                            dialog->num_checked != 0);
+  return visible;
+}
+
... 6 more ...

You have to check if it's NULL first or it will crash if there's a reference cycle.

(Which means there's not much value in using g_clear_object here either.)

@@ +407,3 @@
+  object_class->dispose = clear_data_dialog_dispose;
+
+  g_type_ensure (WEBKIT_TYPE_WEBSITE_DATA);

Why is this required? I see you don't need it in class_init itself, not when setting properties. You're getting a hang without it?

::: src/resources/gtk/clear-data-dialog.ui
@@ +130,3 @@
+                          <item translatable="yes">the past day</item>
+                          <item translatable="yes">the past week</item>
+                          <item translatable="yes">the past 4 weeks</item>

"4" -> "four"
Comment 7 Michael Catanzaro 2017-01-24 04:56:25 UTC
(In reply to Michael Catanzaro from comment #6)
> You have to check if it's NULL first or it will crash if there's a reference
> cycle.
> 
> (Which means there's not much value in using g_clear_object here either.)

Er, something went wrong in splinter. The problem is you call g_cancellable_cancel() in dispose without null-checking the cancellable as required.
Comment 8 Carlos Garcia Campos 2017-01-24 06:41:01 UTC
(In reply to Michael Catanzaro from comment #6)
> Review of attachment 344058 [details] [review] [review]:
> 
> Really nice, it looks great! I prefer you wait until the next WebKit release
> to commit it.

Sure, I'll wait until we have a new version to depend on.

> One bad thing I notice is the type to search doesn't work, the input gets
> captured by the tree view instead of by your search box. So please fix that;
> anywhere the user can see a search button, typing a normal character should
> activate the button and begin a search.

Ah, I didn't know that, I'll check how it's done in other dialogs.

> Another bad thing is that I still have a bunch of application cache files
> left over (including a 10 MB one) after deleting all application cache from
> the beginning of time. But I guess that's what you fixed in
> https://trac.webkit.org/changeset/211035, right?

No, that fix is only for WebSQL databases, what do you mean exactly by application cache files? Offline web application cache? HTTP disk cache? We have unit tests in WebKit to check that data is properly removed.

> The biggest problem is that you can only clear data for particular sites if
> selecting the beginning of time option. I don't care about the restriction,
> but I do about how it's presented to users: there's no way to know that you
> need to select "from the beginning of time" in order to clear data for
> particular sites. So we need something better here. One easy solution would
> be to change the default state of the combo box to "from the beginning of
> time".

Yes, that's how the WebKit2 API works, you can remove data from sites, or clear data for a period of time, but not both. Making the rows insensitive is not clear enough. I thought about making from the beginning of time the default, but then you would be confused when switching to another option anyway, and using last hour is consistent with other browsers. I think we should just explain it in the dialog.

> I'd make one incidental change to the prefs dialog itself: change the "Clear
> personal data..." button to "Manager personal data..." so that users don't
> fear that pressing the button is destructive.

Good idea, it's also consistent with cookies and passwords too.

> If you want bonus points, store in GSettings which of the top-level
> checkboxes are checked: bug #770755.

I'm not sure I agree with that bug, though.

> ::: src/clear-data-dialog.c
> @@ +147,3 @@
> +
> +  treestore = GTK_TREE_STORE (dialog->treestore);
> +  for (guint i = 0; i < G_N_ELEMENTS(data_entries); i++) {
> 
> G_N_ELEMENTS (data_entries)
> 
> @@ +178,3 @@
> +  }
> +
> +  g_list_free_full (data_list, (GDestroyNotify)webkit_website_data_unref);
> 
> Is the cast really needed?

I think so, but I'll check.

> @@ -127,2 +387,9 @@
> -  gtk_widget_set_sensitive (dialog->clear_button,
> -                            dialog->num_checked != 0);
> +  return visible;
> +}
> +
> ... 6 more ...
> 
> You have to check if it's NULL first or it will crash if there's a reference
> cycle.
> 
> (Which means there's not much value in using g_clear_object here either.)

Nope, g_cancellable_cancel is null safe.

> @@ +407,3 @@
> +  object_class->dispose = clear_data_dialog_dispose;
> +
> +  g_type_ensure (WEBKIT_TYPE_WEBSITE_DATA);
> 
> Why is this required? I see you don't need it in class_init itself, not when
> setting properties. You're getting a hang without it?

I'm using WebKitWebsiteData in the ui file, we just need to ensure the type is already registered when gtkbuilder builds the GtkListStore. Static glib types are never unregistered, so we only need to ensure it once, and class init is a good place for that.

> ::: src/resources/gtk/clear-data-dialog.ui
> @@ +130,3 @@
> +                          <item translatable="yes">the past day</item>
> +                          <item translatable="yes">the past week</item>
> +                          <item translatable="yes">the past 4 weeks</item>
> 
> "4" -> "four"

Copied it from chromium :-) But I agree four looks better.
Comment 9 Michael Catanzaro 2017-01-24 14:09:54 UTC
One more thing: can you add some indication that the operation either succeeded or failed? Could be as simple as a GtkMessageDialog with message "Success!" and a button that says "Yay!" in the case of success. Your API has a GError parameter so it should be possible. *The clear data dialog should be non-interactive during this time.*
Comment 10 Michael Catanzaro 2017-01-24 14:16:27 UTC
(In reply to Carlos Garcia Campos from comment #8)
> > Another bad thing is that I still have a bunch of application cache files
> > left over (including a 10 MB one) after deleting all application cache from
> > the beginning of time. But I guess that's what you fixed in
> > https://trac.webkit.org/changeset/211035, right?
> 
> No, that fix is only for WebSQL databases, what do you mean exactly by
> application cache files? Offline web application cache? HTTP disk cache? We
> have unit tests in WebKit to check that data is properly removed.

Offline web application cache. I have in ~/.cache/epiphany/applications a 57.3 kB ApplicationCache.db, a 32.8 ApplicationCache.db-shm, and an 10.4 MB ApplicationCache.db-wal, despite having just deleted all of my offline web application cache since the beginning of time using your dialog.

> Yes, that's how the WebKit2 API works, you can remove data from sites, or
> clear data for a period of time, but not both. Making the rows insensitive
> is not clear enough. I thought about making from the beginning of time the
> default, but then you would be confused when switching to another option
> anyway, and using last hour is consistent with other browsers. I think we
> should just explain it in the dialog.

OK, that works too.
 
> > If you want bonus points, store in GSettings which of the top-level
> > checkboxes are checked: bug #770755.
> 
> I'm not sure I agree with that bug, though.

OK, then please close it. ;)
Comment 11 Carlos Garcia Campos 2017-01-24 15:06:58 UTC
(In reply to Michael Catanzaro from comment #9)
> One more thing: can you add some indication that the operation either
> succeeded or failed? Could be as simple as a GtkMessageDialog with message
> "Success!" and a button that says "Yay!" in the case of success. Your API
> has a GError parameter so it should be possible. *The clear data dialog
> should be non-interactive during this time.*

I don't understand, there's a spinner while getting the data, then the treeview is shown. In case of no data, a message saying there's no data is shown. The API has a GError and returns gboolean but it never fails. I used that approach for consistency with the glib async pattern or in case it fails in the future. But at the moment it can only fail if the cancellable is cancelled, which only happens on dispose.
Comment 12 Carlos Garcia Campos 2017-01-24 15:08:42 UTC
(In reply to Michael Catanzaro from comment #10)
> (In reply to Carlos Garcia Campos from comment #8)
> > > Another bad thing is that I still have a bunch of application cache files
> > > left over (including a 10 MB one) after deleting all application cache from
> > > the beginning of time. But I guess that's what you fixed in
> > > https://trac.webkit.org/changeset/211035, right?
> > 
> > No, that fix is only for WebSQL databases, what do you mean exactly by
> > application cache files? Offline web application cache? HTTP disk cache? We
> > have unit tests in WebKit to check that data is properly removed.
> 
> Offline web application cache. I have in ~/.cache/epiphany/applications a
> 57.3 kB ApplicationCache.db, a 32.8 ApplicationCache.db-shm, and an 10.4 MB
> ApplicationCache.db-wal, despite having just deleted all of my offline web
> application cache since the beginning of time using your dialog.

And is that shown in the list of data fetched? If we fetch that and fail to clear, then file a bug report to WebKit, otherwise it's probably leaked.

> > Yes, that's how the WebKit2 API works, you can remove data from sites, or
> > clear data for a period of time, but not both. Making the rows insensitive
> > is not clear enough. I thought about making from the beginning of time the
> > default, but then you would be confused when switching to another option
> > anyway, and using last hour is consistent with other browsers. I think we
> > should just explain it in the dialog.
> 
> OK, that works too.
>  
> > > If you want bonus points, store in GSettings which of the top-level
> > > checkboxes are checked: bug #770755.
> > 
> > I'm not sure I agree with that bug, though.
> 
> OK, then please close it. ;)

Well, I said I'm not sure.
Comment 13 Michael Catanzaro 2017-01-24 17:09:19 UTC
(In reply to Carlos Garcia Campos from comment #11)
> I don't understand, there's a spinner while getting the data, then the
> treeview is shown. In case of no data, a message saying there's no data is
> shown. The API has a GError and returns gboolean but it never fails. I used
> that approach for consistency with the glib async pattern or in case it
> fails in the future. But at the moment it can only fail if the cancellable
> is cancelled, which only happens on dispose.

I mean the clear operation. Currently when you press clear the dialog disappears immediately, and the user doesn't know if it worked or not.

(In reply to Carlos Garcia Campos from comment #12)
> And is that shown in the list of data fetched? If we fetch that and fail to
> clear, then file a bug report to WebKit, otherwise it's probably leaked.

It was shown before I clicked Clear. Once I clicked Clear, it's no longer shown. So yes, it's been leaked, but it was not leaked until I performed the Clear operation.
Comment 14 Michael Catanzaro 2017-01-24 17:10:02 UTC
(In reply to Michael Catanzaro from comment #13)
> It was shown before I clicked Clear. Once I clicked Clear, it's no longer
> shown. So yes, it's been leaked, but it was not leaked until I performed the
> Clear operation.

To be clear: it disappeared immediately, and did not reappear after restarting Epiphany, so WebKit really cannot see it anymore, even though it's still there on disk.
Comment 15 Carlos Garcia Campos 2017-01-25 06:43:03 UTC
(In reply to Michael Catanzaro from comment #13)
> (In reply to Carlos Garcia Campos from comment #11)
> > I don't understand, there's a spinner while getting the data, then the
> > treeview is shown. In case of no data, a message saying there's no data is
> > shown. The API has a GError and returns gboolean but it never fails. I used
> > that approach for consistency with the glib async pattern or in case it
> > fails in the future. But at the moment it can only fail if the cancellable
> > is cancelled, which only happens on dispose.
> 
> I mean the clear operation. Currently when you press clear the dialog
> disappears immediately, and the user doesn't know if it worked or not.

Ah, it never fails, so I don't want to delay the dialog destruction for nothing.

> (In reply to Carlos Garcia Campos from comment #12)
> > And is that shown in the list of data fetched? If we fetch that and fail to
> > clear, then file a bug report to WebKit, otherwise it's probably leaked.
> 
> It was shown before I clicked Clear. Once I clicked Clear, it's no longer
> shown. So yes, it's been leaked, but it was not leaked until I performed the
> Clear operation.

That's a WebKit bug then. We have a unit test to check exactly that, though.
Comment 16 Michael Catanzaro 2017-01-25 15:13:24 UTC
(In reply to Carlos Garcia Campos from comment #15)
> Ah, it never fails, so I don't want to delay the dialog destruction for
> nothing.

Then the GError parameters are just ignored?

> > (In reply to Carlos Garcia Campos from comment #12)
> > > And is that shown in the list of data fetched? If we fetch that and fail to
> > > clear, then file a bug report to WebKit, otherwise it's probably leaked.
> > 
> > It was shown before I clicked Clear. Once I clicked Clear, it's no longer
> > shown. So yes, it's been leaked, but it was not leaked until I performed the
> > Clear operation.
> 
> That's a WebKit bug then. We have a unit test to check exactly that, though.
Comment 17 Michael Catanzaro 2017-01-25 15:15:30 UTC
(In reply to Carlos Garcia Campos from comment #15)
> Ah, it never fails, so I don't want to delay the dialog destruction for
> nothing.

I think it should be delayed until the function returns anyway. What you can do then is display a GtkMessageDialog that says the operation succeeded, but still add code to Epiphany to display an error dialog as well in case WebKit ever decides to set the error parameter in the future. We have to do something to indicate the operation succeeded; if the dialog just disappears when you click clear, users get suspicious.
Comment 18 Carlos Garcia Campos 2017-01-25 16:23:08 UTC
(In reply to Michael Catanzaro from comment #17)
> (In reply to Carlos Garcia Campos from comment #15)
> > Ah, it never fails, so I don't want to delay the dialog destruction for
> > nothing.
> 
> I think it should be delayed until the function returns anyway. What you can
> do then is display a GtkMessageDialog that says the operation succeeded, but
> still add code to Epiphany to display an error dialog as well in case WebKit
> ever decides to set the error parameter in the future. We have to do
> something to indicate the operation succeeded; if the dialog just disappears
> when you click clear, users get suspicious.

That's what both chromium and firefox do, and I'm sure nobody suspects anything, you just assumes it worked. I bet it's also what our current code does. Not closing the dialog might look like it hung, or showing a spinner just to close the dialog is not ideal either. Showing a stupid popup just to say that data was deleted is annoying, you are supposed to delete the data. So, no, I'm not going to do that.
Comment 19 Carlos Garcia Campos 2017-01-25 16:23:45 UTC
(In reply to Michael Catanzaro from comment #16)
> (In reply to Carlos Garcia Campos from comment #15)
> > Ah, it never fails, so I don't want to delay the dialog destruction for
> > nothing.
> 
> Then the GError parameters are just ignored?
> 

Yes.