1376511 - Browser:Thumbnail:CheckState should be handled during idle period in child process

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Browser:Thumbnail:CheckState handling shows up during non-idle periods when running speedometer.

Comment 1

[Markus Stange [:mstange]]

Here's the whole profile of the first 50 seconds of speedometer, filtered to only samples that include processing this message: http://bit.ly/2sY0ACD

The only time it seems to take up any time at all is the first time it's called, which is when it loads the PageThumbUtils.jsm module. That instance of the handler takes 2.3ms. Unfortunately this first handler falls right into one of the async tests, so it contributes to the speedometer score.
All the other instances of this message take between 0.1ms and 0.3ms.

Comment 2

[Andrew Overholt [:overholt]]

Will this change with Activity Stream?

Comment 3

[Tim Spurway [:tspurway]]

Just cc'ing ursula, who could provide a better answer

Comment 4

[Ursula Sarracini (:ursula)]

I don't think this will change with Activity Stream to be honest - Activity Stream will still attempt to get the thumbnails in the same way that the current about:newtab does, which means it will still be loading PageThumbUtils.jsm. I can look into this further - it might be a good opportunity to deal with Bug 1353584 now, which may help with this here.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

I agree that this code:

http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/toolkit/components/thumbnails/PageThumbUtils.jsm#256-320

could probably be made to use requestIdleCallback, and could be broken up into smaller tasks.

We might also want to have the parent avoid querying the child for this information[1] if we know the tab has become busy again. We do something similar if the tab is closed.

[1]: http://searchfox.org/mozilla-central/rev/b7e531410ebc1c7d74a78e86a894e69608db318c/browser/base/content/browser-thumbnails.js#157-168

Comment 6

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period

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

Comment 7

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8895111 [details]
Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period

My (rather simplistic) approach seems to work (I don't see Browser:Thumbnail:CheckState in my profile anymore). However, I am concerned that I did not really follow your advice from Comment 5. Is this approach ok? Also, how can I tell if it is running too long for an idle callback and needs to be broken up?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8895111 [details]
Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period

https://reviewboard.mozilla.org/r/166226/#review171732

I actually think this is a fine start.

Looking back through the `shouldStoreContentThumbnail` code, I'm less convinced now that we could usefully break that up into smaller chunks. If we _were_ to do that, I'd recommend making the `shouldStoreContentThumbnail` an `async function`, and having it immediately await on a `requestIdleCallback`, and accepting the `IdleDeadline` object that a `requestIdleCallback` gets. Then, as the `shouldStoreContentThumbnail` progresses, check the `IdleDeadline` to see if we've exceeded it. As soon as that happens, we should await on another `requestIdleCallback` and overwrite the `IdleDeadline` with what the callback eventually gets.

However, that sounds like a lot of work given the fact that we don't really have a good sense of how expensive `shouldStoreContentThumbnail` is (we just know it can run at inconvenient times).

So I suggest we take your patch here, and see if the times that `shouldStoreContentThumbnail` is called are problematically long - if so, we can then revisit breaking it up. Sound good?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a3018f82800
Handle Browser:Thumbnail:CheckState during idle period r=mconley

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing browser-chrome's browser_thumbnails_bug726727.js and browser_thumbnails_bug818225.js:

https://hg.mozilla.org/integration/autoland/rev/c40dce56082e54b22e9ec0a81473cb3f31d5f4a3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1a3018f828002b511b693f0508cd90a62175acc8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=122024337&repo=autoland

09:32:01     INFO - TEST-START | toolkit/components/thumbnails/test/browser_thumbnails_bug726727.js
09:32:01     INFO - GECKO(1931) | JavaScript error: chrome://global/content/browser-child.js, line 354: NS_ERROR_PORT_ACCESS_NOT_ALLOWED: Component returned failure code: 0x804b0013 (NS_ERROR_PORT_ACCESS_NOT_ALLOWED) [nsIWebNavigation.loadURIWithOptions]
09:32:01     INFO - GECKO(1931) | JavaScript error: chrome://global/content/browser-child.js, line 562: ReferenceError: requestIdleCallback is not defined
09:32:46     INFO - TEST-INFO | started process screencapture
09:32:46     INFO - TEST-INFO | screencapture: exit 0
09:32:46     INFO - Buffered messages logged at 09:32:01
09:32:46     INFO - Console message: [JavaScript Error: "NS_ERROR_PORT_ACCESS_NOT_ALLOWED: Component returned failure code: 0x804b0013 (NS_ERROR_PORT_ACCESS_NOT_ALLOWED) [nsIWebNavigation.loadURIWithOptions]" {file: "chrome://global/content/browser-child.js" line: 354}]
09:32:46     INFO - Console message: [JavaScript Error: "ReferenceError: requestIdleCallback is not defined" {file: "chrome://global/content/browser-child.js" line: 562}]
09:32:46     INFO - Buffered messages logged at 09:32:10

<telemetry error messages>

09:32:46     INFO - Buffered messages finished
09:32:46     INFO - TEST-UNEXPECTED-FAIL | toolkit/components/thumbnails/test/browser_thumbnails_bug726727.js | Test timed out -

Comment 11

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8895111 [details]
Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period

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

Comment 12

[Robin Steuber (they/them) [:bytesized]]

Hopefully this version of the patch fixes this problem. Pushing to try to confirm.

Comment 13

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8895111 [details]
Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period

Tests seem to be passing on try. Look good to re-merge?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8895111 [details]
Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period

https://reviewboard.mozilla.org/r/166226/#review171882

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9499e1097909
Handle Browser:Thumbnail:CheckState during idle period r=mconley

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9499e1097909