Closed Bug 619487 Opened 11 years ago Closed 8 years ago
Preferences should not be allowed to be accessed off of the main thread
Basically, we need to grab the preferences part of the patch from bug 580790.
And we don't want to assert for now; just warn.
blocking2.0: --- → ?
Yeah, we need this to resolve other blocker-type crashes and such.
blocking2.0: ? → betaN+
Comment on attachment 499347 [details] [diff] [review] patch r=sdwilsh
Attachment #499347 - Flags: review?(sdwilsh) → review+
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
From bug 580790 comment 37: --- Comment 37 John J. Barton 2011-01-09 11:54:01 PST This patch causes messages in Error Console: Invalid use of the preferences on a background thread! These message have no value: there is no call stack, no filename, no line number, and you have to search through the Firefox source code to realize that it is caused by the platform code. Please remove this message or change it to be useful. --- Comment 38 Dan Witte (:dwitte) 2011-01-09 13:47:08 PST Try setting a breakpoint in a debug build? --- Comment 39 Ben Bucksch (:BenB) 2011-01-09 15:19:23 PST > Try setting a breakpoint in a debug build? You're asking JS developers to do that?? Are JS modules affected? I.e. can we use prefs in JS modules at least? I am not sure where they run, but they sure need prefs. Please make preferences thread-safe instead of asking all apps and extensions to adapt.
> Are JS modules affected? Rather, nsITimer, which I need to do delayed execution or intervals in JS modules.
> These message have no value: there is no call stack, no filename, no line > number That's because the code that detects the problem has no way to determine those. > You're asking JS developers to do that?? Yes, if they're managing to hit this problem. Last I checked, there was no way to do from JS directly in Gecko 2.0. You have to write C++ code or find existing buggy C++ code to trigger it. > Please make preferences thread-safe instead of asking all apps and extensions > to adapt. Why? They aren't now, they never have been... Nothing has changed except now we _tell_ you when something is broken instead of it breaking silently. The only "adapting" that needs to happen is that code that has been broken all along should ... be fixed. As for your JS modules and nsITimer question, timers run on the thread they're posted from. That's about all I can say without seeing more of your code.
> there was no way to do from JS directly in Gecko 2.0. > You have to write C++ code or find existing buggy C++ code to trigger it. OK, that would be good. However, we do hit it, with pure JS (and stock FF4 trunk). We're investigating, once the dev has the debug build built.
We're running into PSM bug 624411, which is likely identical with bug 585706. Would have been nice to fix the latter first before spouting the warning :(.
Depends on: 585706
No, I'd much rather bring the issue to prominence in a beta.
I backed this out, because it caused bug 624514, which is holding off beta9. http://hg.mozilla.org/mozilla-central/rev/93a8c5ffa86d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Deassigning. If someone wants to pick this up, feel free!
Assignee: dwitte → nobody
Unless firstname.lastname@example.org comes up with a patch soon, this is going to be wontfix for 2.0... Oh well!
(In reply to comment #14) > Unless email@example.com comes up with a patch soon, this is going to be > wontfix for 2.0... Oh well! Well, the patch here works, it's just that it causes things to break because: 1) nobody checks return values from the preference service 2) people access the pref service off of the main thread Now, one could easily argue that these consumers are broken...
At this point we aren't going to do this for FF4, but we'd still like it done. Any volunteers?
blocking2.0: betaN+ → -
(In reply to comment #16) > At this point we aren't going to do this for FF4, but we'd still like it done. > Any volunteers? Sure, I can land this patch once we branch. Is this going to get backed out again if we find more broken consumers? Will I be on hook for fixing those broken consumers?
Assignee: nobody → sdwilsh
Status: REOPENED → ASSIGNED
This patch cannot land until PSM is fixed. We'll deal with other cases as they come up.
sdwilsh, you could just make it output a warning, without blocking access (i.e. stuff works as before). Maybe also let the warning link to a webpage that explains the issue, and states that JS code cannot cause this (but trigger it), and what to do to fix code that causes it. I do think that the high-profile consumers within Mozilla should be fixed before adding even the warning. This costed us a day of work - including building a current debug build which we usually don't need - to analyze an - as it turned out - already known bug, which we just triggered with our code, by merely using https:, but of course the timing blamed us.
(In reply to comment #19) > sdwilsh, you could just make it output a warning, without blocking access (i.e. > stuff works as before). I don't think that is sensible. That'd be like having a warning label on a bomb that has a random detonator (you can use it, but who knows when it will actually do what you want and not cause havok when you don't want it to). If that's the path we decide to take, someone else can fix this.
> That'd be like having a warning label on a bomb that has a random detonator Seems like that the mine in PSM has not detonated much, and the defusing broke things. Warning-only gives time to fix things without breaking things.
> Seems like that the mine in PSM has not detonated much, Do you have data to back that up?
We have plenty of low-level crashes in pref hashtables which can probably but not certainly be attributed to this set of bugs. But really, we know what needs to happen, and we're not doing it for FF4 now, so let's let sleeping dogs like so we can ship?
This is something I'm interested in picking up again.
10 years ago
Depends on: 714445
10 years ago
Depends on: 714449
No longer depends on: 585706
Depends on: 753656
Depends on: 832677
Assignee: sdwilsh → khuey
Whiteboard: [blocked by 585706, 624514]
Depends on: 832696
Depends on: 842715
No longer depends on: 832677
This works around bug 832677 and makes off-main-thread preferences usage crash.
Comment on attachment 719184 [details] [diff] [review] Patch I think this needs to do something to catch callers going through the nsIPrefService / nsIPrefBranch methods. FWIW, the patch I had was: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/ed89c8769291/pref-main-thread-assertions
Depends on: 910808
Depends on: 910810
Depends on: 910860
Depends on: 910878
Depends on: 946860
Depends on: 946865
Depends on: 946907
Comment on attachment 8386486 [details] [diff] [review] Patch It would also be good to put the assertion in PREF_DeleteBranch, PREF_ClearAllUserPrefs, and pref_HashPref. r=dbaron
Attachment #8386486 - Flags: review?(dbaron) → review+
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/3d87e2db21e3 (along with bug 979951 and bug 968836) for m-bc bustage on at least OSX: https://tbpl.mozilla.org/php/getParsedLog.php?id=35695094&tree=Mozilla-Inbound
(In reply to Wes Kocher (:KWierso) from comment #32) > Backed out in > http://hg.mozilla.org/integration/mozilla-inbound/rev/3d87e2db21e3 (along > with bug 979951 and bug 968836) for m-bc bustage on at least OSX: > https://tbpl.mozilla.org/php/getParsedLog.php?id=35695094&tree=Mozilla- > Inbound Looks like b2g mochitests don't like this at all.
b2g still has issues, I'll file a followup for it. https://hg.mozilla.org/integration/mozilla-inbound/rev/c50ed2757551
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.