Closed
Bug 1425144
Opened 6 years ago
Closed 6 years ago
0.34 severe hangs / khr in CreateFileW while creating jump lists
Categories
(Firefox :: General, defect, P2)
Firefox
General
Tracking
()
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: alexical, Assigned: alexical)
References
Details
(Whiteboard: [bhr-html][fxperf:p1])
Attachments
(2 files)
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
Assignee | ||
Comment 1•6 years ago
|
||
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
Updated•6 years ago
|
Priority: -- → P3
Comment 2•6 years ago
|
||
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]
Comment 3•6 years ago
|
||
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)
Updated•6 years ago
|
Priority: P3 → P1
Comment 4•6 years ago
|
||
I used P1 to mirror [qf:p1], but 61 is the next release, so P2 fits better.
Priority: P1 → P2
Updated•6 years ago
|
Flags: needinfo?(mconley)
Whiteboard: [bhr-html][qf:f61][qf:p1] → [bhr-html][qf:f61][qf:p1][fxperf]
Assignee | ||
Comment 5•6 years ago
|
||
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)
Comment 6•6 years ago
|
||
(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)
Assignee | ||
Updated•6 years ago
|
Whiteboard: [bhr-html][qf:f61][qf:p1][fxperf] → [bhr-html][qf:f61][qf:p1][fxperf:p3]
Comment 7•6 years ago
|
||
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•6 years ago
|
||
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]
Updated•6 years ago
|
Whiteboard: [bhr-html][qf:f61][qf:p1][fxperf:p2] → [bhr-html][qf:f64][qf:p1][fxperf:p2]
Updated•6 years ago
|
Whiteboard: [bhr-html][qf:f64][qf:p1][fxperf:p2] → [bhr-html][qf:p1:f64][fxperf:p2]
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Whiteboard: [bhr-html][qf:p1:f64][fxperf:p2] → [bhr-html][qf:p1:f64][fxperf:p1]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•6 years ago
|
||
Tackling this in a few pieces. InitJumpLists shows up with the greatest percentage of hangs on arewesmoothyet right now, so looking at that first.
Assignee | ||
Updated•6 years ago
|
Attachment #8988611 -
Flags: review?(jmathies) → review?(aklotz)
Assignee | ||
Comment 11•6 years ago
|
||
Moving the reviewer since I think Jim might be a bit swamped. Aaron, do you have time to look at this?
Comment 12•6 years ago
|
||
mozreview-review |
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-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
(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•6 years ago
|
||
mozreview-review |
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 17•6 years ago
|
||
mozreview-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+
Comment 18•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•6 years ago
|
||
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
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/89cfe2dc3cda https://hg.mozilla.org/mozilla-central/rev/b8deff1aeb2d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment 25•6 years ago
|
||
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?
Assignee | ||
Comment 26•6 years ago
|
||
(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•6 years ago
|
||
Filed Bug 1476238, I don't think I'm able to make it block this bug though.
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [bhr-html][qf:p1:f64][fxperf:p1] → [bhr-html][fxperf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•