797120 - Assertion failure: _mOwningThread.GetThread() == PR_GetCurrentThread() (ValueObserver not thread-safe), at /Volumes/2mac/gaia/isrc/modules/libpref/src/Preferences.cpp:131

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Gregor Wagner [:gwagner]]

I see this during shutdown of the b2g desktop build.

Comment 1

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 2

[Gregor Wagner [:gwagner]]

The backtrace:
http://pastebin.mozilla.org/1855092

Comment 3

[Gregor Wagner [:gwagner]]

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

This fixes the problem but I am not 100% sure if this is the right fix.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Review of attachment 667171 [details] [diff] [review]:
-----------------------------------------------------------------

This is absolutely not the right fix. Preferences are not threadsafe and simply declaring them to be won't fix anything, it will just hide bugs.

It looks like something added a preference observer off the main thread. It needs to not do that.

nsPrefBranch::AddObserver and nsPrefBranch::RemoveObserver should assert NS_IsMainThread. That will let you catch this bug when it is actually introduced.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Assertion stack trace

This is the first place where AddObserver is called from the main thread.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Abort on access from the main thread

This is the patch I used to get the log above. I made it abort because I was getting a bazillion assertions just waiting for the homescreen to appear.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Your patch is backwards, you want to abort if you're *not* on the main thread.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Abort on access off the main thread

Doh! This is the version that aborts on access off the main thread.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Assertion stack trace

...and this is the log from running desktop B2G with the above patch.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

mozilla::gl::GLContext::CanUploadNonPowerOfTwo should absolutely not be calling AddBoolVarCache when off the main thread!!!

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: Do caching setup in GLContext::Init

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 676876 [details] [diff] [review]
Do caching setup in GLContext::Init

Review of attachment 676876 [details] [diff] [review]:
-----------------------------------------------------------------

This seems to rely on the assumption that InitWithPrefix is only called on the main thread, which AFAIU isn't the case.

The approach we've used in previous similar bugs was to move such must-happen-on-main-thread initialization things to gfxPlatform. See e.g. how at the beginning of InitWithPrefix we query gfxPlatform for WorkAroundDriverBugs(). Indeed gfxPlatform initialization at least is guaranteed to happen on the main thread.

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

See this patch -- with you reviewed ;-)
https://bugzilla.mozilla.org/attachment.cgi?id=613137&action=diff

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Side note: it would be nice if we could have read-only, no-guarantees-in-case-of-race, access to preferences off the main thread. Not having that is causing many such discussions, and is causing us to move more things to the main thread at startup for no real good reason.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Do caching setup in GLContext::Init

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 678208 [details] [diff] [review]
Do caching setup in GLContext::Init

Review of attachment 678208 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContext.cpp
@@ +641,2 @@
>  
>      if (!sPowerOfTwoPrefCached) {

Seems like this CacheCanUploadNPOT should only ever get called once, from gfxPlatform::Init--->GLContext::PlatformSetup. So you could consider either removing this if(), or replacing it by an assertion confirming that we area only doing this once.

Comment 17

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d54c207490e

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6d54c207490e