Closed Bug 619487 Opened 14 years ago Closed 10 years ago

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

Categories

(Core :: Preferences: Backend, defect)

Product:
Core
Component:
Preferences: Backend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30
Tracking Flags:
Tracking Status
blocking2.0 --- -

People

(Reporter: sdwilsh, Assigned: khuey)

References

Details

Attachments

(1 file, 2 obsolete files)

Patch
3.34 KB, patch
dbaron
: review+
Details | Diff | Splinter Review 
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+
Attached patch patch (obsolete) — — Splinter Review 

        Attachment #499347 -
        Flags: review?(sdwilsh)

    
    

    
  
Comment on attachment 499347 [details] [diff] [review]
patch

r=sdwilsh

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

    
    

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

    
    

    
  
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.

    
  
Depends on: 624315

    
  
Depends on: 624411

    
    

    
  
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.

    
  
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]

    
  
No longer blocks: 624315
Depends on: 624315

    
    

    
  
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!

    
    

    
  
(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]

    
    

    
  
(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?

    
  
Whiteboard: [blocked by 585706, 624514]

    
    

    
  
This is something I'm interested in picking up again.
Depends on: 809363
Assignee: sdwilsh → khuey
Whiteboard: [blocked by 585706, 624514]

    
    

    
  
PSM is good, but we now have bug 832677 and bug 832696 to deal with before we can flip the switch.

    
  
Blocks: 836263

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (obsolete)
        — 
          — Splinter Review
      

    


  
This works around bug 832677 and makes off-main-thread preferences usage crash.

        Attachment #499347 -
        Attachment is obsolete: true

        Attachment #719184 -
        Flags: review?(dbaron)

    
    

    
  
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)

    
    

    
  
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+

    
    

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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/c50ed2757551
Status: ASSIGNED → RESOLVED
Closed: 14 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: