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

RESOLVED FIXED in Firefox 57

Status

()

Firefox
New Tab Page
RESOLVED FIXED
2 months ago
8 days ago

People

(Reporter: smaug, Assigned: bytesized)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
Browser:Thumbnail:CheckState handling shows up during non-idle periods when running speedometer.
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.
Will this change with Activity Stream?
Flags: needinfo?(tspurway)
Just cc'ing ursula, who could provide a better answer
Flags: needinfo?(tspurway) → needinfo?(usarracini)
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.
Flags: needinfo?(usarracini)
Whiteboard: [qf] → [qf:p1]
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
(Assignee)

Updated

10 days ago
Assignee: nobody → ksteuber
Comment hidden (mozreview-request)
(Assignee)

Comment 7

10 days ago
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?
Attachment #8895111 - Flags: feedback?(mconley)

Comment 8

9 days ago
mozreview-review
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?
Attachment #8895111 - Flags: review+
Attachment #8895111 - Flags: feedback?(mconley)

Comment 9

9 days ago
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a3018f82800
Handle Browser:Thumbnail:CheckState during idle period r=mconley
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 -
Flags: needinfo?(ksteuber)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

9 days ago
Hopefully this version of the patch fixes this problem. Pushing to try to confirm.
Flags: needinfo?(ksteuber)
(Assignee)

Comment 13

9 days ago
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?
Attachment #8895111 - Flags: review?(mconley)

Comment 14

9 days ago
mozreview-review
Comment on attachment 8895111 [details]
Bug 1376511 - Handle Browser:Thumbnail:CheckState during idle period

https://reviewboard.mozilla.org/r/166226/#review171882
Attachment #8895111 - Flags: review?(mconley) → review+

Comment 15

9 days ago
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9499e1097909
Handle Browser:Thumbnail:CheckState during idle period r=mconley

Comment 16

8 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9499e1097909
Status: NEW → RESOLVED
Last Resolved: 8 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.