1354143 - JumpListBuilder::CommitListBuild does main thread IO

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Florian Quèze [:florian]]

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

[(no longer active)]

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

[(no longer active)]

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...

Comment 5

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

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.

Comment 6

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: 0001-_-test.patch

WIP for XPCOM side.
Need to change jsm and test cases.

Comment 7

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: 0001-_-xpcom.patch

Uploaded wrong patch.

Comment 8

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: Bug 1354143 - Commit jump list on lazy idle thread.

Since committing will do IO on the main thread, it would be better to do it on
an idle thread instead. We have to change JavaScript code too because now the
API is asynchrous.

This patch also updates its xpcshell test.

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

Comment 9

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8871648 [details]
Bug 1354143 - Commit jump list on lazy idle thread.

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

Comment 10

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8871648 [details]
Bug 1354143 - Commit jump list on lazy idle thread.

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

Comment 11

[Florian Quèze [:florian]]

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

[Jim Mathies [:jimm]]

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'

Comment 13

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

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?

Comment 14

[Florian Quèze [:florian]]

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.

Comment 15

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8871648 [details]
Bug 1354143 - Commit jump list on lazy idle thread.

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

Comment 16

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

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

Comment 17

[Florian Quèze [:florian]]

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.

Comment 18

[Florian Quèze [:florian]]

Jim, could you please have a quick look at the new changes to always call NS_GetSpecialDirectory on the main thread?

Comment 19

[Jim Mathies [:jimm]]

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 20

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8871648 [details]
Bug 1354143 - Commit jump list on lazy idle thread.

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ce78807187a
Commit jump list on lazy idle thread. r=florian,jimm

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1ce78807187a