1022421 - AsyncDeleteAllFaviconsFromDisk::Run() uses prefs off the main thread

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

I just got a fatal assertion:

Assertion failure: NS_IsMainThread(), at c:\Users\cpearce\src\mozilla\purple\modules\libpref\src\Preferences.cpp:410

Callstack:

xul.dll!mozilla::Preferences::InitStaticMembers() Line 410	C++
xul.dll!mozilla::Preferences::GetInt(const char * aPref=0x1a33fa38, int * aResult=0x1a33fa18) Line 1362	C++
xul.dll!mozilla::Preferences::GetInt(const char * aPref=0x1a33fa38, int aDefault=86400) Line 116	C++
xul.dll!mozilla::widget::FaviconHelper::GetICOCacheSecondsTimeout() Line 1196	C++
xul.dll!mozilla::widget::AsyncDeleteAllFaviconsFromDisk::Run() Line 1012	C++
xul.dll!nsThread::ProcessNextEvent(bool aMayWait=false, bool * aResult=0x1a33fc7f) Line 766	C++

It seems that AsyncDeleteAllFaviconsFromDisk::Run() is being dispatched to a non-main thread here:
http://mxr.mozilla.org/mozilla-central/source/widget/windows/JumpListBuilder.cpp#395

But it's using the pref service inside FaviconHelper::GetICOCacheSecondsTimeout(), which is main thread only.

I was watching a video in Firefox (just open the video as a new tab) when this happened, about a minute in without causing any keyboard/mouse input, so I'd guess the idleservice kicked in.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch

I kept hitting this, so here's a fix: cache the value of FaviconHelper::GetICOCacheSecondsTimeout() in AsyncDeleteAllFaviconsFromDisk's ctor, so that we don't call FaviconHelper::GetICOCacheSecondsTimeout() off main thread and end up using the pref service and then fatally asserting.

Comment 2

[Jim Mathies [:jimm]]

Comment on attachment 8436576 [details] [diff] [review]
Patch

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

::: widget/windows/WinUtils.cpp
@@ +961,5 @@
>  AsyncDeleteAllFaviconsFromDisk::
>    AsyncDeleteAllFaviconsFromDisk(bool aIgnoreRecent)
>    : mIgnoreRecent(aIgnoreRecent)
>  {
> +  mIcoNoDeleteSeconds = FaviconHelper::GetICOCacheSecondsTimeout() + 600;

please add a comment explaining why we cache this.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5947370612ff

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5947370612ff