Closed Bug 1425144 Opened 4 years ago Closed 3 years ago

0.34 severe hangs / khr in CreateFileW while creating jump lists

Categories

(Firefox :: General, defect, P2)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: dthayer, Assigned: dthayer)

References

Details

(Whiteboard: [bhr-html][qf:p1:f64][fxperf:p1])

Attachments

(2 files)

Bug 1425144 - Init Jump list on lazy idle thread
59 bytes, text/x-review-board-request
aklotz
: review+
Details
Bug 1425144 - Init Jump list on lazy idle thread (js)
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
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
Priority: -- → P3
Panos, the QF team triage this bug as important to fix for F61.  Can you please look into getting resource to work on this?
Flags: needinfo?(past)
Whiteboard: [bhr-html][qf] → [bhr-html][qf:f61][qf:p1]
Mike, should we move this to Widget:Win32 or do you think this is something your team can work on?
Flags: needinfo?(past) → needinfo?(mconley)
Priority: P3 → P1
I used P1 to mirror [qf:p1], but 61 is the next release, so P2 fits better.
Priority: P1 → P2
Flags: needinfo?(mconley)
Whiteboard: [bhr-html][qf:f61][qf:p1] → [bhr-html][qf:f61][qf:p1][fxperf]
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?
Flags: needinfo?(jmathies)
(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.
Flags: needinfo?(jmathies)
Whiteboard: [bhr-html][qf:f61][qf:p1][fxperf] → [bhr-html][qf:f61][qf:p1][fxperf:p3]
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).
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.
Whiteboard: [bhr-html][qf:f61][qf:p1][fxperf:p3] → [bhr-html][qf:f61][qf:p1][fxperf:p2]
Whiteboard: [bhr-html][qf:f61][qf:p1][fxperf:p2] → [bhr-html][qf:f64][qf:p1][fxperf:p2]
Whiteboard: [bhr-html][qf:f64][qf:p1][fxperf:p2] → [bhr-html][qf:p1:f64][fxperf:p2]
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Whiteboard: [bhr-html][qf:p1:f64][fxperf:p2] → [bhr-html][qf:p1:f64][fxperf:p1]
Tackling this in a few pieces. InitJumpLists shows up with the greatest percentage of hangs on arewesmoothyet right now, so looking at that first.
Attachment #8988611 - Flags: review?(jmathies) → review?(aklotz)
Moving the reviewer since I think Jim might be a bit swamped. Aaron, do you have time to look at this?
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.
Attachment #8988611 - Flags: review?(aklotz) → review-
(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 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
```
Attachment #8991145 - Flags: review?(gijskruitbosch+bugs) → review+
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
Attachment #8988611 - Flags: review?(aklotz) → review+
(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)
Pushed by dothayer@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/89cfe2dc3cda
https://hg.mozilla.org/mozilla-central/rev/b8deff1aeb2d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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?
(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.
Filed Bug 1476238, I don't think I'm able to make it block this bug though.
Depends on: 1476238
Depends on: 1489317
See Also: → 1529276
You need to log in before you can comment on or make changes to this bug.