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 695828 - Profile migration in 3.7.91 eats CPU
Profile migration in 3.7.91 eats CPU
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: General
3.7.x
Other Linux
: Normal normal
: ---
Assigned To: Epiphany Maintainers
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-03-14 09:17 UTC by Bastien Nocera
Modified: 2013-03-19 18:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ephy-profile-migrator: Stop spinning when porting form passwords (3.27 KB, patch)
2013-03-14 09:17 UTC, Bastien Nocera
committed Details | Review

Description Bastien Nocera 2013-03-14 09:17:02 UTC
The CPU keeps spinning, and the passwords aren't getting migrated.

  • #0 pthread_mutex_lock
    from /lib64/libpthread.so.0
  • #1 g_mutex_lock
    from /lib64/libglib-2.0.so.0
  • #2 g_main_context_prepare
    from /lib64/libglib-2.0.so.0
  • #3 g_main_context_iterate.isra.22
    from /lib64/libglib-2.0.so.0
  • #4 g_main_context_iteration
    from /lib64/libglib-2.0.so.0
  • #5 migrate_form_passwords_to_libsecret
    at ephy-profile-migrator.c line 944
  • #6 ephy_migrator
    at ephy-profile-migrator.c line 1009
  • #7 main
    at ephy-profile-migrator.c line 1077
  • #5 migrate_form_passwords_to_libsecret
    at ephy-profile-migrator.c line 944
$1 = 368

Instead of spinning and using g_main_context_iteration(), you should create a mainloop which you'd exit from the callback when there are no more passwords to handle.
Comment 1 Bastien Nocera 2013-03-14 09:17:28 UTC
Created attachment 238849 [details] [review]
ephy-profile-migrator: Stop spinning when porting form passwords

UNTESTED

Use atomic operations to manipulate the counter that tracks the number
of operations we need to achieve, and use our own main loop rather
than spinning on g_main_context_iteration().

This also fixes the number of operations not decreasing when
ephy_form_auth_data_store() fails.
Comment 2 Bastien Nocera 2013-03-14 09:25:19 UTC
Running the test by hand, I get some errors like:
** (ephy-profile-migrator:22720): WARNING **: Couldn't store a form password: Timeout was reached

Which shows that the error path bug in ephy_form_auth_data_store() might be the culprit. Doesn't explain the error though...
Comment 3 Claudio Saavedra 2013-03-19 17:19:12 UTC
(In reply to comment #2)

> Which shows that the error path bug in ephy_form_auth_data_store() might be the
> culprit. Doesn't explain the error though...

That error seems to come from libsecret/gdbus. The best we can do is to gracefully handle it. Since in this case we depend on an external service to run the migration successfully we can't guarantee that it will properly work, so we need to review the migrator not to leave ephy in a half-broken state.
Comment 4 Claudio Saavedra 2013-03-19 17:22:06 UTC
(In reply to comment #1)
> Created an attachment (id=238849) [details] [review]
> ephy-profile-migrator: Stop spinning when porting form passwords
> 
> UNTESTED
> 
> Use atomic operations to manipulate the counter that tracks the number
> of operations we need to achieve, and use our own main loop rather
> than spinning on g_main_context_iteration().

I am not sure why the atomic operations are needed if the manipulation of this integer happens always in the mainloop?

I agree that using our own mainloop is better, though.

> This also fixes the number of operations not decreasing when
> ephy_form_auth_data_store() fails.

And this is definitely the reason why the migrator never ends.
Comment 5 Claudio Saavedra 2013-03-19 17:39:10 UTC
Review of attachment 238849 [details] [review]:

::: lib/ephy-profile-migrator.c
@@ +857,3 @@
+  if (g_atomic_int_dec_and_test (&form_passwords_migrating))
+    g_main_loop_quit (loop);
+

The problem with this approach is that if the service fails to store a password and we complete the migration then the user will not have a chance to recover that password. If the service is down or breaks entirely, for any reason, this might mean no password migration whatsoever.

I'm slightly inclined to prefer the migrator to fail here with a message indicating that the secret service is probably down or something like that.
Comment 6 Bastien Nocera 2013-03-19 17:52:10 UTC
The patch was over-engineered, but:
1) using a mainloop fixes all the CPU being burnt (even this error doesn't happen)
2) the g_atomic_int* stuff is necessary if you're not certain all the callbacks will happen in the same thread (they might do, I didn't double-check all the code paths)
3) the missing error handler was the cause of the never-ending process

I'm happy splitting those up for patching if you're up for it.
Comment 7 Claudio Saavedra 2013-03-19 18:22:37 UTC
Yes, I got that. Well, it's a tough call between not migrating the passwords if the service is not available/timing out and aborting ephy ad infinitum. Let's go for the former hope the users don't miss passwords if there's a failure.

Please commit after removing the UNTESTED warning; no need to split them.
Comment 8 Bastien Nocera 2013-03-19 18:46:27 UTC
Attachment 238849 [details] pushed as 7eaba58 - ephy-profile-migrator: Stop spinning when porting form passwords