Blocking preference write causes us to re-enter event loop

RESOLVED FIXED in Firefox 57

Status

()

Core
Preferences: Backend
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: milan, Assigned: milan)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
All
Unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 unaffected, firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
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.
(Assignee)

Updated

4 months ago
Assignee: nobody → milan
See Also: → bug 1386263, bug 789945
(Assignee)

Comment 1

4 months ago
We are using nsIEventTarget::Dispatch with DISPATCH_SYNC flag.  We should have been using SyncRunnable::DispatchToThread instead.
(Assignee)

Comment 2

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10d425f9f71e80bf9eafb4a3fb0064d482ad3691
Comment hidden (mozreview-request)

Comment 4

4 months ago
mozreview-review
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
Attachment #8895073 - Flags: review?(bugs) → review+
(Assignee)

Comment 5

4 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a4ad52e7105e2c03acfcad1a3773585e69c641 actually.
(Assignee)

Comment 6

4 months ago
NI Benjamin just to make sure he's seen this.
Flags: needinfo?(benjamin)

Comment 7

4 months ago
Pushed by msreckovic@mozilla.com:
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
(Assignee)

Updated

4 months ago
See Also: → bug 1384454
(Assignee)

Updated

4 months ago
Blocks: 1384454
(Assignee)

Updated

4 months ago
See Also: bug 1384454

Comment 8

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6ffe8a4a92c7
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Updated

4 months ago
Flags: needinfo?(benjamin)

Comment 9

4 months ago
Didn't this code land for 56? I think this needs an uplift.
status-firefox56: --- → ?
tracking-firefox56: --- → ?
Flags: needinfo?(milan)
(Assignee)

Comment 10

4 months ago
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.
Flags: needinfo?(milan)
Track 56+ in case we will enable the pref.
tracking-firefox56: ? → +
(Assignee)

Comment 12

3 months ago
This is still disabled by pref in 56, so 56 is unaffected.
status-firefox56: ? → unaffected
tracking-firefox56: + → ---
Blocks: 1386263
Duplicate of this bug: 1387242
You need to log in before you can comment on or make changes to this bug.