GNOME Bugzilla – Bug 695828
Profile migration in 3.7.91 eats CPU
Last modified: 2013-03-19 18:46:32 UTC
The CPU keeps spinning, and the passwords aren't getting migrated.
+ Trace 231642
$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.
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.
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...
(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.
(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.
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.
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.
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.
Attachment 238849 [details] pushed as 7eaba58 - ephy-profile-migrator: Stop spinning when porting form passwords