JumpListBuilder::CommitListBuild does main thread IO

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Shell Integration
RESOLVED FIXED
3 months ago
9 days 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])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 months 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

3 months ago
Blocks: 1338595
Whiteboard: [qf] → [qf:p1]
(Reporter)

Updated

2 months ago
See Also: → bug 1357114
(Reporter)

Comment 1

2 months ago
Here is a profile on the quantum reference hardware where this caused a 732ms pause on the UI thread: https://perfht.ml/2oQJ4Bu
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>
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...

Updated

2 months ago
Whiteboard: [qf:p1] → [qf:p1][bhr]
See Also: → bug 1310281
Duplicate of this bug: 1310281
Blocks: 1364015
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.
Created attachment 8870738 [details] [diff] [review]
0001-_-test.patch

WIP for XPCOM side.
Need to change jsm and test cases.
Created attachment 8870740 [details] [diff] [review]
0001-_-xpcom.patch

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

26 days 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

25 days 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

17 days 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

15 days 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

15 days 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

15 days 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

13 days 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

12 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1ce78807187a
Status: NEW → RESOLVED
Last Resolved: 12 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Updated

9 days ago
Flags: needinfo?(jmathies)
You need to log in before you can comment on or make changes to this bug.