Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Preferences should not be allowed to be accessed off of the main thread

RESOLVED FIXED in mozilla30

Status

()

Core
Preferences: Backend
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: sdwilsh, Assigned: khuey)

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Basically, we need to grab the preferences part of the patch from bug 580790.
(Reporter)

Comment 1

7 years ago
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 3

7 years ago
Created attachment 499347 [details] [diff] [review]
patch
Attachment #499347 - Flags: review?(sdwilsh)
(Reporter)

Comment 4

7 years ago
Comment on attachment 499347 [details] [diff] [review]
patch

r=sdwilsh
Attachment #499347 - Flags: review?(sdwilsh) → review+

Comment 5

7 years ago
http://hg.mozilla.org/mozilla-central/rev/d2856d5970b6
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Depends on: 623522

Comment 6

7 years ago
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.

Comment 7

7 years ago
> Are JS modules affected?

Rather, nsITimer, which I need to do delayed execution or intervals in JS
modules.

Comment 8

7 years ago
> 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.

Comment 9

7 years ago
> 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.
(Reporter)

Updated

7 years ago
Depends on: 624315

Updated

7 years ago
Depends on: 624411

Comment 10

7 years ago
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.
(Reporter)

Updated

7 years ago
Blocks: 624490
Blocks: 624315
No longer depends on: 624315
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 → ---
Whiteboard: [softblocker]

Updated

7 years ago
No longer blocks: 624315
Depends on: 624315
Depends on: 624514

Comment 13

7 years ago
Deassigning. If someone wants to pick this up, feel free!
Assignee: dwitte → nobody
Unless nobody@mozilla.org comes up with a patch soon, this is going to be wontfix for 2.0...  Oh well!
(Reporter)

Comment 15

7 years ago
(In reply to comment #14)
> Unless nobody@mozilla.org 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+ → -
Whiteboard: [softblocker]
(Reporter)

Comment 17

7 years ago
(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.

Comment 19

7 years ago
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.
(Reporter)

Comment 20

7 years ago
(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.

Comment 21

7 years ago
> 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.

Comment 22

7 years ago
> 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?
(Reporter)

Updated

6 years ago
Whiteboard: [blocked by 585706, 624514]

Updated

6 years ago
Duplicate of this bug: 669363
This is something I'm interested in picking up again.
Depends on: 714445
Depends on: 714449
No longer depends on: 585706
Depends on: 753656

Updated

5 years ago
Depends on: 809363
Depends on: 832677
Assignee: sdwilsh → khuey
Whiteboard: [blocked by 585706, 624514]
Depends on: 832696
PSM is good, but we now have bug 832677 and bug 832696 to deal with before we can flip the switch.
Blocks: 836263
Depends on: 842715
No longer depends on: 832677
Created attachment 719184 [details] [diff] [review]
Patch

This works around bug 832677 and makes off-main-thread preferences usage crash.
Attachment #499347 - Attachment is obsolete: true
Attachment #719184 - Flags: review?(dbaron)
Attachment #719184 - Flags: review?(bent.mozilla)
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
Attachment #719184 - Attachment is obsolete: true
Attachment #719184 - Flags: review?(dbaron)
Attachment #719184 - Flags: review?(bent.mozilla)
Depends on: 910808
Depends on: 910810
Depends on: 910860
Depends on: 910878
Depends on: 946860
Depends on: 946865
Depends on: 946907
Depends on: 974010
Created attachment 8386486 [details] [diff] [review]
Patch
Attachment #8386486 - Flags: review?(dbaron)
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9535abc58cd6
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
https://hg.mozilla.org/mozilla-central/rev/c50ed2757551
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Blocks: 980419
Blocks: 949325
Blocks: 1204405
You need to log in before you can comment on or make changes to this bug.