1388464 - Blocking preference write causes us to re-enter event loop

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

See 1386263.  The goal of the sync pref file save off the main thread was to block the main thread until the work is done, not to allow an event loop to spin which is allowed to process other events.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

We are using nsIEventTarget::Dispatch with DISPATCH_SYNC flag.  We should have been using SyncRunnable::DispatchToThread instead.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=10d425f9f71e80bf9eafb4a3fb0064d482ad3691

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1388464: Use SyncRunnable instead of DISPATCH_SYNC to actually block the main thread during the blocking preference file write.

Review commit: https://reviewboard.mozilla.org/r/166198/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166198/

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8895073 [details]
Bug 1388464: Use SyncRunnable instead of DISPATCH_SYNC to actually block the main thread during the blocking preference file write.

https://reviewboard.mozilla.org/r/166198/#review171466

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a4ad52e7105e2c03acfcad1a3773585e69c641 actually.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

NI Benjamin just to make sure he's seen this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ffe8a4a92c7
Use SyncRunnable instead of DISPATCH_SYNC to actually block the main thread during the blocking preference file write.  r=smaug

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ffe8a4a92c7

Comment 9

[Benjamin Smedberg]

Didn't this code land for 56? I think this needs an uplift.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Indeed, but 56 has it disabled through a preference, so we shouldn't hit this codepath.  But, yes, if we want to enable the off main thread preference write and on pref change auto save, we'd need this patch as well.

Comment 11

[Gerry Chang [:gchang]]

Track 56+ in case we will enable the pref.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

This is still disabled by pref in 56, so 56 is unaffected.