890618 - FaviconHelper::GetOutputIconPath causes main thread jank

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Honza Bambas (:mayhemer)]

ntdll.dll!_NtQueryFullAttributesFile@8()  + 0x12 bytes	
 	ntdll.dll!_NtQueryFullAttributesFile@8()  + 0x12 bytes	
 	xul.dll!nsAString_internal::SetCapacity(unsigned int capacity=0, const mozilla::fallible_t & __formal={...})  Line 546	C++
 	xul.dll!GetFileInfo(const nsString & name={...}, PRFileInfo64 * info=0x00000000)  Line 690 + 0xe bytes	C++
 	xul.dll!nsLocalFile::ResolveAndStat()  Line 1048 + 0xc bytes	C++
 	xul.dll!nsLocalFile::Create(unsigned int type=1, unsigned int attributes=511)  Line 1232	C++
>	xul.dll!mozilla::widget::FaviconHelper::GetOutputIconPath(nsCOMPtr<nsIURI> aFaviconPageURI={...}, nsCOMPtr<nsIFile> & aICOFile={...}, bool aURLShortcut=false)  Line 917 + 0xf bytes	C++
 	xul.dll!mozilla::widget::FaviconHelper::ObtainCachedIconFile(nsCOMPtr<nsIURI> aFaviconPageURI={...}, nsString & aICOFilePath={...}, nsCOMPtr<nsIThread> & aIOThread={...}, bool aURLShortcut=false)  Line 819 + 0x26 bytes	C++
 	xul.dll!mozilla::widget::JumpListShortcut::GetShellLink(nsCOMPtr<nsIJumpListItem> & item={...}, nsRefPtr<IShellLinkW> & aShellLink={...}, nsCOMPtr<nsIThread> & aIOThread={...})  Line 410 + 0x44 bytes	C++
 	xul.dll!mozilla::widget::JumpListBuilder::AddListToBuild(short aCatType=3, nsIArray * items=0x105925f0, const nsAString_internal & catName={...}, bool * _retval=0x004ee388)  Line 361 + 0x17 bytes	C++

Comment 1

[Brian R. Bondy [:bbondy]]

I think we can move the directory create inside AsyncEncodeAndWriteIcon::Run()

Comment 2

[Brian R. Bondy [:bbondy]]

Hi Even, would you be interested in helping with this bug?
We want the directory creation calls shown in the call stack of Comment 0 out of the main thread and instead in the thread.  See WinUtils.cpp

This line needs to be moved out of GetOutputIconPath and called later:
http://dxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.cpp#l981

Comment 3

[Arun Khetarpal]

(In reply to Brian R. Bondy [:bbondy] from comment #2)
> Hi Even, would you be interested in helping with this bug?
> We want the directory creation calls shown in the call stack of Comment 0
> out of the main thread and instead in the thread.  See WinUtils.cpp
> 
> This line needs to be moved out of GetOutputIconPath and called later:
> http://dxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.
> cpp#l981

Hi Brian: 

I would love to help out with this bug, are you up for mentoring?

Comment 4

[Brian R. Bondy [:bbondy]]

(In reply to akhetarp from comment #3)
> (In reply to Brian R. Bondy [:bbondy] from comment #2)
> > Hi Even, would you be interested in helping with this bug?
> > We want the directory creation calls shown in the call stack of Comment 0
> > out of the main thread and instead in the thread.  See WinUtils.cpp
> > 
> > This line needs to be moved out of GetOutputIconPath and called later:
> > http://dxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.
> > cpp#l981
> 
> Hi Brian: 
> 
> I would love to help out with this bug, are you up for mentoring?

Awesome! Absolutely.

Have you built the source code yet? If so See Comment 2 for a hint on where to start work on the problem.  

I'm off this week by the way, so I may be a bit slow to respond and not on IRC. But just ask questions here and I'll help out. Or you can email me if you prefer [e-mail].

Comment 5

[Honza Bambas (:mayhemer)]

Any updates here?

Comment 6

[Brian R. Bondy [:bbondy]]

Resetting back for someone to take it, I haven't heard back. I'll reach out to a contributor.

Comment 7

[Brian R. Bondy [:bbondy]]

This is the create directory call:
http://dxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.cpp#1070

We want to instead put the call in this function:
http://dxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.cpp#797

Comment 8

[Anuj Agarwal [:anujagarwal464]]

Attachment: create directory removed from main thread

Comment 9

[Brian R. Bondy [:bbondy]]

Comment on attachment 8389324 [details] [diff] [review]
create directory removed from main thread

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

One small change but this looks good, put the next patch for review and I'll do some testing locally and if that passes then I'll r+ the patch! Nice work!

::: widget/windows/WinUtils.cpp
@@ +837,5 @@
>    NS_ENSURE_TRUE(icoFile, NS_ERROR_FAILURE);
>    rv = icoFile->InitWithPath(mIconPath);
>  
> +  // Try to create the directory if it's not there yet
> +  nsCOMPtr<nsIFile> aICOFile;

The a prefix on aICOFile is normally only used for function parameters.
Let's call this:
nsCOMPtr<nsIFile> dirPath;

Comment 10

[Anuj Agarwal [:anujagarwal464]]

Attachment: create directory removed from main thread - updated directory name

Comment 11

[Brian R. Bondy [:bbondy]]

Comment on attachment 8389345 [details] [diff] [review]
create directory removed from main thread - updated directory name

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

I tried this locally and it's working fine. I also put a breakpoint on the new code and confirmed it's off the main thread now. Nice work :)

Comment 12

[Brian R. Bondy [:bbondy]]

I pushed it to try, assuming all tests still pass I'll push it to mozilla-inbound next, then within a day it'll be on mozilla-central.
https://tbpl.mozilla.org/?tree=Try&rev=30b6a1fca67d

Comment 13

[Honza Bambas (:mayhemer)]

I checked the patch too, looks pretty good to me.  Thanks!

Comment 14

[Anuj Agarwal [:anujagarwal464]]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> I checked the patch too, looks pretty good to me.  Thanks!

:D :D

Comment 15

[Anuj Agarwal [:anujagarwal464]]

(In reply to Brian R. Bondy [:bbondy] from comment #12)
> I pushed it to try, assuming all tests still pass I'll push it to
> mozilla-inbound next, then within a day it'll be on mozilla-central.
> https://tbpl.mozilla.org/?tree=Try&rev=30b6a1fca67d

Build failed on Windows XP Opt.

Comment 16

[Brian R. Bondy [:bbondy]]

Probably just an intermittent failure unrelated to this.  I'll keep an eye on it.

Comment 17

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/354865950bba

Comment 18

[Paul Silaghi, QA [:pauly]]

Does this need manual QA verification ?

Comment 19

[Brian R. Bondy [:bbondy]]

Yes please, just make sure jump lists still work in the same way they did before.  You may want to also uninstall and delete your profile before testing so you have to create the jump list folder which this code changes.

Comment 20

[Paul Silaghi, QA [:pauly]]

31.0a2 (2014-05-08), 32.0a1 (2014-05-08) Win 7 x64
(In reply to Brian R. Bondy [:bbondy] from comment #19)
> You may want to also uninstall and delete your profile before
> testing so you have to create the jump list folder which this code changes.
Exactly on the first run after doing this, the jump list won't show up. But this is not a regression of this bug, it's happening on a pre-fix build too. Restarting FF and opening again couple of tabs make the jump list to show up.

On a regular browsing, the jump list is properly displayed/updated after ~ 2 min.
I'm marking this bug as verified. If you think the above issue should be treated separately, please say and I'll file a new bug.

Comment 21

[Brian R. Bondy [:bbondy]]

Yep that's fine, we don't try to generate the ICOs on startup on purpose because it would lead to a perf hit on startup.