1425144 - 0.34 severe hangs / khr in CreateFileW while creating jump lists

Status: RESOLVED FIXED

(Firefox :: General, defect, P2)

Description

[Alex Thayer [:alexical] (she/her)]

Category: Hangs longer than 2048ms
Process: default
Thread: Gecko
Hang time: 1.717 ms / hr
Number of hangs: 0.0003365 hangs / hr
Stack:
    CreateFileW
    CEnumFiles::_InitEnumeration(unsigned short const *,unsigned short const *,int,tagWIN32_FIND_DATA_EX *)
    CFSFolder::ParseDisplayName(HWND__ *,IBindCtx *,unsigned short *,unsigned long *,_ITEMIDLIST_RELATIVE * *,unsigned long *)
    CDrivesFolder::ParseDisplayName(HWND__ *,IBindCtx *,unsigned short *,unsigned long *,_ITEMIDLIST_RELATIVE * *,unsigned long *)
    CRegFolder::ParseDisplayName(HWND__ *,IBindCtx *,unsigned short *,unsigned long *,_ITEMIDLIST_RELATIVE * *,unsigned long *)
    CDesktopFolder::ParseDisplayName(HWND__ *,IBindCtx *,unsigned short *,unsigned long *,_ITEMIDLIST_RELATIVE * *,unsigned long *)
    CRegFolder::ParseDisplayName(HWND__ *,IBindCtx *,unsigned short *,unsigned long *,_ITEMIDLIST_RELATIVE * *,unsigned long *)
    SHParseDisplayName
    ILCreateFromPathEx
    CShellLink::_SetPIDLPath(_ITEMIDLIST_ABSOLUTE const *,unsigned short const *,SLSPPFLAGS)
    CShellLink::SetPath(unsigned short const *)
    mozilla::widget::JumpListShortcut::GetShellLink(nsCOMPtr<nsIJumpListItem> &,RefPtr<IShellLinkW> &,nsCOMPtr<nsIThread> &)
    mozilla::widget::JumpListBuilder::AddListToBuild(short,nsIArray *,nsTSubstring<char16_t> const &,bool *)
    XPTC__InvokebyIndex
    XPCWrappedNative::CallMethod(XPCCallContext &,XPCWrappedNative::CallMode)
    XPC_WN_CallMethod(JSContext *,unsigned int,JS::Value *)
    js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct)
    Interpret
    js::RunScript(JSContext *,js::RunState &)
    /modules/WindowsJumpLists.jsm:264
    js::InternalCallOrConstruct(JSContext *,JS::CallArgs const &,js::MaybeConstruct)
    js::jit::InvokeFunction(JSContext *,JS::Handle<JSObject *>,bool,bool,unsigned int,JS::Value *,JS::MutableHandle<JS::Value>)
    js::jit::InvokeFromInterpreterStub(JSContext *,js::jit::InterpreterStubExitFrameLayout *)
    <unsymbolicated>
    /modules/WindowsJumpLists.jsm:289
    /modules/WindowsJumpLists.jsm:419
    XREMain::XRE_main

Comment 1

[Alex Thayer [:alexical] (she/her)]

Note: this was created looking at https://arewesmoothyet.com.

For reference:
https://arewesmoothyet.com/?callTreeFilters=prefix-0123456789abcdefgy1y2y3y4Nfy5y6&category=all&durationSpec=2048_65536&mode=explore&payloadID=02190fe153ce4a80829a66ddef54d00e&thread=2

Comment 2

[Jean Gong :jgong]

Panos, the QF team triage this bug as important to fix for F61.  Can you please look into getting resource to work on this?

Comment 3

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

Mike, should we move this to Widget:Win32 or do you think this is something your team can work on?

Comment 4

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

I used P1 to mirror [qf:p1], but 61 is the next release, so P2 fits better.

Comment 5

[Alex Thayer [:alexical] (she/her)]

Jim, since I'm not too familiar with this am I right in understanding that this is this run when right clicking Firefox in the task bar on Windows? Would the proposed solution be running this in a background thread, which would presumably delay populating the jump list by the same time but not hang FF itself? Is that possible / is that how the user would experience it?

Comment 6

[Jim Mathies [:jimm]]

(In reply to Doug Thayer [:dthayer] from comment #5)
> Jim, since I'm not too familiar with this am I right in understanding that
> this is this run when right clicking Firefox in the task bar on Windows?
> Would the proposed solution be running this in a background thread, which
> would presumably delay populating the jump list by the same time but not
> hang FF itself? Is that possible / is that how the user would experience it?

This doesn't trigger in response to user action. I think we're doing everything we can to push this procedure out. We delay it on startup and only refresh once every 30 seconds or so. I think setting the actual data needs to happen on the ui thread, should confirm that if we're not using a background thread already.

Comment 7

[Florian Quèze [:florian]]

In the general case on the JS side this is started from a 120s timer + and idle callback: https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/browser/modules/WindowsJumpLists.jsm#521

Apparently when changing a pref, we update twice, once from a 120s timer https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/browser/modules/WindowsJumpLists.jsm#530 and once from an idlecallback created immediately 2 lines later: https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/browser/modules/WindowsJumpLists.jsm#532

We do this immediately (without timer or idle callback) in the browser:purge-session-history case or at shutdown when clearing history at shutdown: https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/browser/modules/WindowsJumpLists.jsm#167 (although hopefully in the shutdown case we always update to an empty list so we shouldn't hit the specific stack from comment 0).

Comment 8

[Mike Conley (:mconley) (:⚙️)]

This appears pretty consistently in arewesmoothyet.com, and if it is potentially running every 2 mins, that seems pretty bad.

I'm upping the priority - let's get this fixed soonish.

Comment 9

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1425144 - Init Jump list on lazy idle thread

We're already committing jump lists on mIOThread, and I see
no errors or problems with initting on mIOThread either, so
I think this is okay. Had to restructure some things to
make using a Promise simple, and to avoid dispatching to
mIOThread from within mIOThread (since we can only dispatch
to a LazyIdleThread from the owner thread).

Review commit: https://reviewboard.mozilla.org/r/253850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253850/

Comment 10

[Alex Thayer [:alexical] (she/her)]

Tackling this in a few pieces. InitJumpLists shows up with the greatest percentage of hangs on arewesmoothyet right now, so looking at that first.

Comment 11

[Alex Thayer [:alexical] (she/her)]

Moving the reviewer since I think Jim might be a bit swamped. Aaron, do you have time to look at this?

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8988611 [details]
Bug 1425144 - Init Jump list on lazy idle thread

https://reviewboard.mozilla.org/r/253850/#review262676

I think you should find another reviewer to take on the JS bits, if possible. I do not have any familiarity with that code.

::: widget/nsIJumpListBuilder.idl:109
(Diff revision 1)
>    /**
>     * Initializes a jump list build and returns a list of items the user removed
>     * since the last time a jump list was committed. Removed items can become state
>     * after initListBuild is called, lists should be built in single-shot fasion.
>     *
>     * @param removedItems

Please update these comment to reflect the interface change.

::: widget/windows/JumpListBuilder.cpp:207
(Diff revision 1)
> +    return result.StealNSResult();
> +  }
>  
> +  // Hold a strong reference to self, but pass this for convenience
> +  RefPtr<JumpListBuilder> self(this);
> +  nsCOMPtr<nsIRunnable> runnable(NS_NewRunnableFunction("InitListBuild",

I'm not super comfortable with using a non-trivial lambda here; there's too much context.

Please replace this lambda with a new, private JumpListBuilder method and use NewRunnableMethod instead.

::: widget/windows/JumpListBuilder.cpp:223
(Diff revision 1)
> +      }));
> +      return;
> +    }
>  
> -  if(sBuildingList)
> +    nsTArray<nsString> urisToRemove;
> +    if (!NS_FAILED(rv)) {

s/!NS_FAILED/NS_SUCCEEDED/

::: widget/windows/JumpListBuilder.cpp:228
(Diff revision 1)
> +    if (!NS_FAILED(rv)) {
> +      if(sBuildingList) {
> -    AbortListBuild();
> +        AbortListBuild();
> +      }
>  
> -  IObjectArray *objArray;
> +      IObjectArray *objArray;

Let's change this to use RefPtr

::: widget/windows/JumpListBuilder.cpp:545
(Diff revision 1)
> -  objArray->GetCount(&count);
> +  aObjArray->GetCount(&count);
> -
> -  nsCOMPtr<nsIJumpListItem> item;
>  
>    for (uint32_t idx = 0; idx < count; idx++) {
>      IShellLinkW * pLink = nullptr;

Let's change this to use RefPtr

::: widget/windows/JumpListBuilder.cpp:547
(Diff revision 1)
>  
>    for (uint32_t idx = 0; idx < count; idx++) {
>      IShellLinkW * pLink = nullptr;
> -    IShellItem * pItem = nullptr;
>  
> -    if (SUCCEEDED(objArray->GetAt(idx, IID_IShellLinkW, (LPVOID*)&pLink))) {
> +    if (SUCCEEDED(aObjArray->GetAt(idx, IID_IShellLinkW, (LPVOID*)&pLink))) {

I'd suggest changing this block to:
```
if (FAILED(aObjArray->GetA(...)) {
  continue;
}
```

And then you can get rid of one level of indentation for the successful case.

::: widget/windows/JumpListBuilder.cpp:565
(Diff revision 1)
> -    }
> +      }
>  
> -    if (pLink)
> -      pLink->Release();
> -    if (pItem)
> -      pItem->Release();
> +      int iconIdx = 0;
> +      hres = pLink->GetIconLocation(buf, MAX_PATH, &iconIdx);
> +      if (SUCCEEDED(hres)) {
> +        nsString spec(buf);

This can be a nsDependentString

::: widget/windows/JumpListBuilder.cpp:579
(Diff revision 1)
> +  MOZ_ASSERT(!NS_IsMainThread());
>  
> -    if (NS_SUCCEEDED(rv)) {
> -      removedItems->AppendElement(item);
> +  // Check that we aren't deleting some arbitrary file that is not an icon
> +  if (StringTail(aPath, 4).LowerCaseEqualsASCII(".ico")) {
> +    // Construct the parent path of the passed in path
> +    nsCOMPtr<nsIFile> icoFile = do_CreateInstance("@mozilla.org/file/local;1");

Just use NS_NewLocalFile here

::: widget/windows/JumpListBuilder.cpp:590
(Diff revision 1)
> +      return;
> +    }
> +
> +    // Check if the cached ICO file exists
> +    bool exists;
> +    rv = icoFile->Exists(&exists);

Any reason why we shouldn't just call Remove regardless of whether or not the file exists? It seems to me like we could save some disk activity.

Comment 13

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1425144 - Init Jump list on lazy idle thread (js)

This is just the js side of the equation for initting jump lists
on a background thread. It should just be functionally equivalent
to the old code with the exception that it works asynchronously
on a promise of an array of urls rather than an nsIMutableArray.

Review commit: https://reviewboard.mozilla.org/r/256112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256112/

Comment 14

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8988611 [details]
Bug 1425144 - Init Jump list on lazy idle thread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253850/diff/1-2/

Comment 15

[Alex Thayer [:alexical] (she/her)]

(In reply to Aaron Klotz [:aklotz] from comment #12)
> I think you should find another reviewer to take on the JS bits, if
> possible. I do not have any familiarity with that code.

Gijs, would you mind just taking a quick look at this? Sorry, I can't find a great reviewer for this code other than Jim Mathies, but I think he's a bit busy. It felt like a pretty mechanical translation though.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8991145 [details]
Bug 1425144 - Init Jump list on lazy idle thread (js)

https://reviewboard.mozilla.org/r/256112/#review263066

LGTM apart from the nit. And yay, less xpcom-y mutablearray goop!

::: browser/modules/WindowsJumpLists.jsm:431
(Diff revision 1)
> -          let uriSpec = oldItem.app.getParameter(0);
> -          URIsToRemove.push(Services.io.newURI(uriSpec));
> -        } catch (err) { }
> +        return Services.io.newURI(spec);
> +      } catch (e) {
> +        return null;
> -      }
> -    }
> +      }
> +    }).filter(uri => uri != null);

Nitty nit (our style guide says not to compare to null/false/undefined because it bites, I agree with that, but I wish we enforced it with eslint to help save time):

```
uri => !!uri
```

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8988611 [details]
Bug 1425144 - Init Jump list on lazy idle thread

https://reviewboard.mozilla.org/r/253850/#review263182

r=me once these two issues are addressed

::: widget/windows/JumpListBuilder.cpp:211
(Diff revision 2)
> +    NewRunnableMethod<RefPtr<Promise>>("InitListBuild",
> +                                       this,
> +                                       &JumpListBuilder::DoInitListBuild,
> +                                       promise);
> +  nsresult rv = mIOThread->Dispatch(runnable, NS_DISPATCH_NORMAL);
> +  NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE_SUCCESS is deprecated. Please replace.

::: widget/windows/JumpListBuilder.cpp:549
(Diff revision 2)
> -  objArray->GetCount(&count);
> +  aObjArray->GetCount(&count);
> -
> -  nsCOMPtr<nsIJumpListItem> item;
>  
>    for (uint32_t idx = 0; idx < count; idx++) {
>      IShellLinkW * pLink = nullptr;

RefPtr please

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Aaron Klotz [:aklotz] from comment #17)
> Comment on attachment 8988611 [details]
> Bug 1425144 - Init Jump list on lazy idle thread
> 
> https://reviewboard.mozilla.org/r/253850/#review263182
> 
> RefPtr please

(To clarify, I am specifically referring to the IShellLinkW interface pointer here)

Comment 19

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8991145 [details]
Bug 1425144 - Init Jump list on lazy idle thread (js)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256112/diff/1-2/

Comment 20

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8988611 [details]
Bug 1425144 - Init Jump list on lazy idle thread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253850/diff/2-3/

Comment 21

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8991145 [details]
Bug 1425144 - Init Jump list on lazy idle thread (js)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256112/diff/2-3/

Comment 22

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8988611 [details]
Bug 1425144 - Init Jump list on lazy idle thread

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253850/diff/3-4/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89cfe2dc3cda
Init Jump list on lazy idle thread (js) r=Gijs
https://hg.mozilla.org/integration/autoland/rev/b8deff1aeb2d
Init Jump list on lazy idle thread r=aklotz

Comment 24

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/89cfe2dc3cda
https://hg.mozilla.org/mozilla-central/rev/b8deff1aeb2d

Comment 25

[Winson Wen]

Just reinstalled Windows and Nightly wasn't adding the "Open new tab / new window / private window" options in the jump list. Installing an old Nightly version fixed it. Is this bug possibly related?

Comment 26

[Alex Thayer [:alexical] (she/her)]

(In reply to winson.wen1 from comment #25)
> Just reinstalled Windows and Nightly wasn't adding the "Open new tab / new
> window / private window" options in the jump list. Installing an old Nightly
> version fixed it. Is this bug possibly related?

Possibly, yes. If you wouldn't mind filing a bug about this that blocks this bug, I will take a look in the morning.

Comment 27

[Winson Wen]

Filed Bug 1476238, I don't think I'm able to make it block this bug though.