JumpListBuilder::CommitListBuild does main thread IO

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: wcpan)

Tracking

(Blocks 2 bugs)

unspecified
Firefox 55
x86_64
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1][bhr])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

2 years ago
See this profile: https://perfht.ml/2o6OzKf

JumpListBuilder::CommitListBuild (called on the UI thread by JS code at http://searchfox.org/mozilla-central/rev/b8cce081200129a1307e2a682f121135b5ca1d19/browser/modules/WindowsJumpLists.jsm#269) calls a Windows API that calls FlushFileBuffers.

The code in JumpListBuilder.cpp already uses an I/O thread, so hopefully the amount of effort to move this Windows API call off the main thread will be reasonable.
Reporter

Updated

2 years ago
Blocks: UIJank

Updated

2 years ago
Whiteboard: [qf] → [qf:p1]
Reporter

Updated

2 years ago
See Also: → 1357114
Reporter

Comment 1

2 years ago
Here is a profile on the quantum reference hardware where this caused a 732ms pause on the UI thread: https://perfht.ml/2oQJ4Bu

Comment 2

2 years ago
This also appears in the data from long running hangs from BHR that are captured stack traces after a period of not processing events after 8 seconds, see:

https://people-mozilla.org/~mlayzell/bhr/20170405/125.html
https://people-mozilla.org/~mlayzell/bhr/20170405/1433.html
https://people-mozilla.org/~mlayzell/bhr/20170405/1221.html

This should be the pseudo-stack for hangs associated with WindowsJumpLists.jsm: <https://people-mozilla.org/~mlayzell/bhr/20170405/35.html>

Comment 3

2 years ago
This bug is pretty scary.  It's not immediately obvious to me if the APIs listed in https://msdn.microsoft.com/en-us/library/windows/desktop/dd378402(v=vs.85).aspx are possible to call off the main thread.  If that turns out to be possible then there is our JS bindings to these APIs that is main-thread only, so we'd need to proxy to a background thread of some sort.  Another option may be to move this work off to a separate process, but that sounds like a lot of work too.

It would be nice to get someone assigned to this bug sooner than later to start to get an idea on what our options are...
Whiteboard: [qf:p1] → [qf:p1][bhr]

Updated

2 years ago
Duplicate of this bug: 1310281
Looks like mJumpListMgr->CommitList() can be used on mIOThread. At least it returns S_OK, and the jump list updated as well.

I'll working on an off-main-thread version, which means we have to change the APIs to async-style though.
Posted patch 0001-_-test.patch (obsolete) — Splinter Review
WIP for XPCOM side.
Need to change jsm and test cases.
Posted patch 0001-_-xpcom.patch (obsolete) — Splinter Review
Uploaded wrong patch.
Attachment #8870738 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Attachment #8870740 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee: nobody → wpan
Reporter

Comment 11

2 years ago
mozreview-review
Comment on attachment 8871648 [details]
Bug 1354143 - Commit jump list on lazy idle thread.

https://reviewboard.mozilla.org/r/143164/#review147510

::: browser/modules/WindowsJumpLists.jsm:269
(Diff revision 3)
>      }
>      return false;
>    },
>  
>    _commitBuild: function WTBJL__commitBuild() {
> -    if (!this._hasPendingStatements() && !this._builder.commitListBuild()) {
> +    return new Promise((resolve) => {

Is anything using the returned promise here?

If not you can simplify this _commitBuild method significantly.

::: browser/modules/WindowsJumpLists.jsm:275
(Diff revision 3)
> +      if (this._hasPendingStatements()) {
> +        resolve(false);
> +        return;
> +      }
> +
> +      this._builder.commitListBuild((succeed) => {

nit: when using an arrow function with a single parameter, we usually don't put parens around it.

Comment 12

2 years ago
mozreview-review
Comment on attachment 8871648 [details]
Bug 1354143 - Commit jump list on lazy idle thread.

https://reviewboard.mozilla.org/r/143164/#review147876

::: widget/windows/JumpListBuilder.cpp:87
(Diff revision 3)
> +    // Destructor does not always call on the main thread.
> +    MOZ_ASSERT(!mCallback);
> +    MOZ_ASSERT(!mBuilder);
> +  }
> +
> +  void Destory()

nit - 'Destroy'
Attachment #8871648 - Flags: review?(jmathies) → review+
After this change, the xpcshell test always fail because it may shutdown threads without profile-before-change. (Similar to bug 1240227)

So I added do_get_profile() to ensure that happens before xpcom-shutdown-threads, but I got this assertion:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=674b45b89c5fc11eb20a432a35e0bf842d04dd6b

Looks like NS_GetSpecialDirectory called some JS code if the environment is xpcshell, which is different from the normal case (i.e. desktop browser).

Am I doing something wrong for this test case?
Is it possible to do the test in xpcshell, or we can disable this test in xpcshell?
Flags: needinfo?(florian)
Reporter

Comment 14

2 years ago
NS_GetSpecialDirectory is being called by mozilla::widget::AsyncDeleteAllFaviconsFromDisk::Run() [widget/windows/WinUtils.cpp:1470]. Is it possible to do the NS_GetSpecialDirectory call once on the main thread before we start doing async stuff on other threads, and keep the result so that we don't need to call this again from other threads?

I think this failure existed before your patch but we were not seeing it because the test was synchronous and finished before this async code had a chance to run.
Flags: needinfo?(florian)
Comment hidden (mozreview-request)
I'm not sure why I got NS_ERROR_FAILURE in xpcshell when I do dirSvc.get("ProfLDS", Ci.nsIFile), but I think it would be better to do that by mozilla::widget::AsyncDeleteAllFaviconsFromDisk itself.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7575341f08299d2770cd3fdef04431e74ec212d
Reporter

Comment 17

2 years ago
mozreview-review
Comment on attachment 8871648 [details]
Bug 1354143 - Commit jump list on lazy idle thread.

https://reviewboard.mozilla.org/r/143164/#review151816

Looks good to me, but I'm not fully comfortable to review the C++ changes, so I would like if jimm could have a final quick look at the latest version of the patch.
Attachment #8871648 - Flags: review?(florian) → review+
Reporter

Comment 18

2 years ago
Jim, could you please have a quick look at the new changes to always call NS_GetSpecialDirectory on the main thread?
Flags: needinfo?(jmathies)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8871648 [details]
Bug 1354143 - Commit jump list on lazy idle thread.

https://reviewboard.mozilla.org/r/143164/#review152004

::: widget/nsIJumpListBuilder.idl:146
(Diff revision 4)
>  
>    /**
>     * Commits the current jump list build to the Taskbar.
>     *
> -   * @returns true if the operation completed successfully.
> +   * @param callback
> +   *        Takes one argument, which is true if the operation completed

s/Takes/Receives
Comment hidden (mozreview-request)

Comment 21

2 years ago
Pushed by wpan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1ce78807187a
Commit jump list on lazy idle thread. r=florian,jimm

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ce78807187a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

2 years ago
Flags: needinfo?(jmathies)
See Also: → 1365643
See Also: → 1372597
Reporter

Updated

2 years ago
Depends on: 1376760

Updated

2 years ago
Depends on: 1376924
You need to log in before you can comment on or make changes to this bug.