[JavaScript Error: "ReadOnlyError: A mutation operation was attempted in a READ_ONLY transaction." {file: "resource://gre/modules/SettingsRequestManager.jsm" line: 509}] No specific STR. I encountered this error several times while using settings app. After the error occurred, all mozSettings requests do not return (both onsuccess and onerror).

This one is all me. I'm seeing it too, though I don't have a good STR yet either. Will add more debug and post once I get it nailed down.

[Blocking Requested - why for this release]: Looks like we're making some transactions as readonly when they're not. This could break existing apps that expect to just work even with a new settings API in place.

Ok. Of course, now that I try and repro this, both on desktop and phone, I can't seem to. Doing all sorts of stuff to settings, no luck reproing the message (debug builds, b2g desktop and flame). Have definitely seen it happen in the past few days though and will leave this open for a few more days. Will also try on release builds.

Well, the good news is, I figured out what's up. The bad news is, it's a somewhat non-trivial fix. When building the settings API, the incorrect assumption was made that one message manager = one app, and to base our permissions on that. There's a couple of cases where this doesn't work. First off, we still do most of our desktop testing on in-process instances, meaning (I think) everything shares a common cpmm in the parent process. This can lead to race conditions that look like: - System app (Settings: readwrite) opens, creates a lock, SettingsRequestManager caches permissions - Calendar app (Settings: readonly) opens, creates a lock, SettingsRequestManager updates permissions for cpmm, which is also system's message manager - System app tries to use its lock, which has to open a new transaction, but now the permissions are readonly due to Calendar, so it opens a read only transaction. It tries to run a set() call on this transaction, and that's when we see the error listed in the bug description. The above steps can race in all sorts of odd ways since our communication is happening over IPC and through a bunch of promises, which may open/close IDB transactions as needed. While I believe this problem has also been seen on the phone, I'm still not exactly sure how we'd achieve this issue in an OOP instance, since as far as I'm aware we only have one instance where we run 2 apps in the same process (settings/bluetooth), and that's being removed. The fix is to remove the message manager caching for permissions, and do a principal check on every received message to make sure the app the message is coming from has the rights we expect. This should future proof us for when we can deal with embedded apps in OOP, also.

WIP at the moment because I want a full try before this gets into reviews.

Comment on attachment 8483920 [details] [diff] [review] Patch 1 (v1) - Make SettingsRequestManager always check principals for permissions Green'd on try, so putting this in for review. Note that this does /not/ solve the problem of multiple principals behind a single message manager dealing with observer notification permissions, but that was already a problem in the first place and I'll file a followup on it.

Comment on attachment 8483920 [details] [diff] [review] Patch 1 (v1) - Make SettingsRequestManager always check principals for permissions Review of attachment 8483920 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty straightforward to me!

Note that due to recent policy changes, all patches must have approval for uplift regardless of blocking status. Please request aurora approval on this patch when you get a chance. Sorry for the inconvenience :(

Comment on attachment 8483920 [details] [diff] [review] Patch 1 (v1) - Make SettingsRequestManager always check principals for permissions Approval Request Comment [Feature/regressing bug #]: Bug 900551 [User impact if declined]: Settings API calls will intermittently fail [Describe test coverage new/current, TBPL]: Integration tests [Risks and why]: Change to settings permissions system, but should be more secure than before [String/UUID change made/needed]: None

Marking NO_UPLIFT due to bug 1064228.

(In reply to Jason Smith [:jsmith] from comment #14) > Marking NO_UPLIFT due to bug 1064228. Dependency is fixed, so marking ready for uplift. Note - we'll need to uplift the dependencies with this bug to ensure there aren't any fallouts.