Markus Stange [:mstange] Reporter
	Description • 9 years ago

For Firefox OS, bug 1154231 added infrastructure to retain layer trees of hidden TabChild instances, up to a number that is controlled with the pref layers.compositor-lru-size (which is set to 10 on B2G and to 0 everywhere else).
This does the following: It keeps a list of up to 10 hidden retained tabs, and once the 11th tab is hidden (which is not in this list), the least recently used tab gets evicted, and gets ClearCachedResources called on it. With a cache size of 0, ClearCachedResources is called from every TabChild::MakeHidden invocation directly.

We could try using that infrastructure for keeping the layer contents of a small number of background tabs around. This would require more memory than before, but we can cap the amount of memory by setting the cache size to a very small number (like 5), and it would make tab switching instant for retained tabs.

However, using it will require changes to our tab switcher. At the moment, if you want to switch back to a tab that was previously visible, we only do that once we've received both a LayerTreeCleared and a LayerTreeReady event for it. (This is to make sure that the LayerTreeReady event isn't coming from a paint that was in flight before the tab was hidden.) But when we retain layer trees for tabs, we won't get a LayerTreeCleared event. So we'd need a different way of ensuring that the LayerTreeReady event we get is the one we want.

	Markus Stange [:mstange] Reporter
	Comment 1 • 9 years ago

Comment 0 sounds a little confusing, now that I read it again. To clarify: In the first part I was talking about B2G (where we use a TabParent / TabChild for each app, I think), and in the last two paragraphs I was referring to Desktop Firefox with e10s.

	Benoit Girard (:BenWa)
	Comment 2 • 9 years ago

My 2 cents: I like the idea but ATM I think our tab switch performance is significantly slower than it should be. We should fix that before we start applying this kind of optimization that would put a band-aid over a serious performance problem.

	Markus Stange [:mstange] Reporter
	Comment 3 • 9 years ago

I agree. We should only do this once we've fixed the other bugs that contribute to slow tab switching.

	Mike Conley (:mconley) (:⚙️)
	Comment 4 • 8 years ago

Re-nomming for triage, since we might want to do this sooner rather than later.

	George Wright (:gw280) (needinfo me!)
	Comment 5 • 8 years ago

ni? to myself to see what the scope of work is here.

	Benjamin Smedberg
	Comment 6 • 8 years ago

Thought: it is likely valuable to give pinned tabs priority

	George Wright (:gw280) (needinfo me!)
	Comment 7 • 8 years ago

I've spoken to Markus about this, and he reckons we can do this in tabbrowser instead of down in the compositor. It'll give us a little more control as well.

I think the best way to do this is to reuse the machinery from the DocShellIsActive property. Basically, I think the way we're binning our layers right now is by setting DocShellIsActive to false when we switch away from a tab/browser/docshell.

I implemented additional machinery to be able to set a DocShell as inactive but retain its layers in the case of minimising in bug 1098131. I propose we reuse that in this way:

- When we switch away from a tab, push the tab we're switching away from onto a queue.
- Set DocShellIsActiveAndForeground = false on the tab we're switching away from.
- If the queue size is larger than the number of tabs we want to keep around, pop the oldest item and set DocShellIsActive = false (which will discard its layer tree).

Question: how large should the queue be?

	Mike Conley (:mconley) (:⚙️)
	Comment 8 • 8 years ago

(In reply to George Wright (:gw280) (:gwright) from comment #7)
> I've spoken to Markus about this, and he reckons we can do this in
> tabbrowser instead of down in the compositor. It'll give us a little more
> control as well.
> 
> I think the best way to do this is to reuse the machinery from the
> DocShellIsActive property. Basically, I think the way we're binning our
> layers right now is by setting DocShellIsActive to false when we switch away
> from a tab/browser/docshell.
> 
> I implemented additional machinery to be able to set a DocShell as inactive
> but retain its layers in the case of minimising in bug 1098131. I propose we
> reuse that in this way:
> 
> - When we switch away from a tab, push the tab we're switching away from
> onto a queue.
> - Set DocShellIsActiveAndForeground = false on the tab we're switching away
> from.
> - If the queue size is larger than the number of tabs we want to keep
> around, pop the oldest item and set DocShellIsActive = false (which will
> discard its layer tree).
> 

Yeah, that all sounds reasonable.

> Question: how large should the queue be?

We actually think we know this! billm landed a Telemetry probe TAB_SWITCH_CACHE_POSITION a few weeks back in bug 1242013.

Here's the histogram:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-02-29&keys=__none__!__none__!__none__&max_channel_version=nightly%252F47&measure=TAB_SWITCH_CACHE_POSITION&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-02-13&table=0&trim=1&use_submission_date=0

This suggests that a cache of about 12 slots should account for 95% of tab switches.

[1]: See https://bugzilla.mozilla.org/show_bug.cgi?id=1242013#c16 for details

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 9 • 6 years ago

George, do you mind if I take this off you and look into what it might involve?

Also, Markus, do you know of anything that's changed related to this? I imagine not, but I figured I'd check in. I think some of this might be a little bit easier given the tab warming work that mconley has already done.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 10 • 6 years ago

George isn't at Mozilla any more, I don't think he'll mind if you take it.

	Markus Stange [:mstange] Reporter
	Comment 11 • 6 years ago

I'm not aware of any changes in this area, other than the work that Mike did. He's a better source of information about this than I am, especially since we decided that this should be implemented in the tab switcher and not in the compositor.

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 12 • 6 years ago

Built a proof of concept for this. I figured I should open up the question again of how much memory do we want to spend on this. On a 2560x1440 display, with Firefox maximized, each cached YouTube seems to eat about 25MB. Napkin math checks out as far as I can tell - 2460 cols * 1440 rows * 4 bytes per pixel = 14.7MB.

This would mean a 12-page cache would cost us 300MB, and a 5-page cache would cost us 125MB. Neither of these figures are trivial, but neither is the thought of up to 95% reduction in tab switch spinners. Thoughts?

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 13 • 6 years ago

(In reply to Doug Thayer [:dthayer] from comment #12)
> Built a proof of concept for this. I figured I should open up the question
> again of how much memory do we want to spend on this. On a 2560x1440
> display, with Firefox maximized, each cached YouTube seems to eat about
> 25MB. Napkin math checks out as far as I can tell - 2460 cols * 1440 rows *
> 4 bytes per pixel = 14.7MB.
> 
> This would mean a 12-page cache would cost us 300MB, and a 5-page cache
> would cost us 125MB. Neither of these figures are trivial, but neither is
> the thought of up to 95% reduction in tab switch spinners. Thoughts?

Woops, forgot to make clear that of course the average display size isn't 2460 * 1440. When I scale the window down to eyeball a 1366*768 display, I see a roughly 33% decrease in the memory per tab. Also, these are all very rough numbers, if that wasn't clear, but I think they serve well enough to illustrate the trade-off.

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 14 • 6 years ago

Markus, I'm trying to look into whether we can limit the cache to a concrete size, rather than an estimated size using a number of tabs. This would require us to have a means of knowing the number of bytes each tab's layers require. As far as I can tell there's no existing mechanism for this, and what we would have to do is compute it using rect sizes of layers in the compositor process and send it up when we SendObserveLayerUpdate(...).

At a very rough level is that correct / feasible? And if so do you have any notes / suggestions?

	Markus Stange [:mstange] Reporter
	Comment 15 • 6 years ago

That sounds right but it also sounds rather complex. I don't really have an opinion on this, and don't have a good grasp of how much payoff we're expecting from the caching and how much complexity it warrants. I'm going to ask around in the gfx team for opinions.

	Markus Stange [:mstange] Reporter
	Comment 16 • 6 years ago

Do you think it would be a good idea to implement this the naive way first, with a configurable number of cached tabs, and then gather some telemetry numbers to see how much it helps? Once we have the numbers, we could still choose to be smarter about how many tabs we keep around before we let the feature ride the trains. On the other hand, if it turns out that this approach doesn't give us much benefit (now that we have tab warming), we won't have wasted any time implementing the smarter solution.

	Mike Conley (:mconley) (:⚙️)
	Comment 17 • 6 years ago

I like the idea of prototyping something simple in AsyncTabSwitcher.jsm, just to see how this feels in practice, and get a sense of the memory cost.

Other things we might consider on top of this:

1) Instead of caching the layer tree as is, suppress the displayport and then flatten the layers into a single bitmap. That means that given a given window size, we can make pretty decent estimates on max cache size (using similar napkin math from comment 12 and comment 13)

2) Consider compressing the bitmaps to reduce the memory overhead. This is, of course, trading space for time again, and operates on the assumption that decompressing a cached image is faster than a full-blown re-paint and upload.

	Mike Conley (:mconley) (:⚙️)
	Comment 19 • 6 years ago
mozreview-review

Comment on attachment 8971032 [details]
Bug 1176019 - Cache layers of background tabs

https://reviewboard.mozilla.org/r/239754/#review245924

Hey dthayer,

This looks great, thanks! I just have one (rather long) worry down below regarding memory. Happy to re-review if there's something I'm missing, or if my worries are unfounded!

::: browser/base/content/tabbrowser.js:100
(Diff revision 1)
>  
>    _lastFindValue: "",
>  
>    _contentWaitingCount: 0,
>  
> +  _tabLayerCache: [],

This makes me a tiny bit antsy, because I can imagine situations where background tabs close, and we never switch tabs (creating an AsyncTabSwitcher), and then we keep these old tabs alive because they exist in the cache.

Ideally, what we'd have here is some kind of ordered collection of weak tabs. Perhaps instead of an Array of <xul:tab>'s, we could use Cu.getWeakReference, and have an Array of nsIWeakReference's... then if an AsyncTabSwitcher comes alive, we can count on it going through the cache list in preActions, and dropping the nsIWeakReference's that .get() to null.

It's a trade, I guess. We're trading keeping a <xul:tab> around in the cache, to keeping an nsIWeakReference around in the cache. I think I'd prefer that to avoid weird leak conditions where <xul:browser>'s still exist for some reason.

The other complication with using an array of nsIWeakReferene's is that it makes checking for the presence of a tab in the queue more cumbersome. I guess given the size of the cache, I'm not sure how much that matters, tbh. If it does matter, we could perhaps either have a WeakSet alongside the Array to check for presence in the cache, OR perhaps use a WeakSet in place of the Array, and use the scarily named ChromeUtils.nondeterministicGetWeakSetKeys[1] to check for order (and delete and add in order to promote the tab's rank).

How do you feel about all of this? Am I worrying too much about memory?

Also, as a follow-up, we might want to observe memory events[2], and clear the layer cache somehow if we're in a low memory situation.

[1]: https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/dom/chrome-webidl/ChromeUtils.webidl#58-68
[2]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIMemory#Low_memory_notifications

::: browser/base/content/tabbrowser.js:2717
(Diff revision 1)
>        if (aTab.closing || (!timedOut && !permitUnload)) {
>          return false;
>        }
>      }
>  
> +    aTab.closing = true;

Can you mention why this needs to be here to avoid leaks in a comment?

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 20 • 6 years ago
mozreview-review-reply

Comment on attachment 8971032 [details]
Bug 1176019 - Cache layers of background tabs

https://reviewboard.mozilla.org/r/239754/#review245924

> This makes me a tiny bit antsy, because I can imagine situations where background tabs close, and we never switch tabs (creating an AsyncTabSwitcher), and then we keep these old tabs alive because they exist in the cache.
> 
> Ideally, what we'd have here is some kind of ordered collection of weak tabs. Perhaps instead of an Array of <xul:tab>'s, we could use Cu.getWeakReference, and have an Array of nsIWeakReference's... then if an AsyncTabSwitcher comes alive, we can count on it going through the cache list in preActions, and dropping the nsIWeakReference's that .get() to null.
> 
> It's a trade, I guess. We're trading keeping a <xul:tab> around in the cache, to keeping an nsIWeakReference around in the cache. I think I'd prefer that to avoid weird leak conditions where <xul:browser>'s still exist for some reason.
> 
> The other complication with using an array of nsIWeakReferene's is that it makes checking for the presence of a tab in the queue more cumbersome. I guess given the size of the cache, I'm not sure how much that matters, tbh. If it does matter, we could perhaps either have a WeakSet alongside the Array to check for presence in the cache, OR perhaps use a WeakSet in place of the Array, and use the scarily named ChromeUtils.nondeterministicGetWeakSetKeys[1] to check for order (and delete and add in order to promote the tab's rank).
> 
> How do you feel about all of this? Am I worrying too much about memory?
> 
> Also, as a follow-up, we might want to observe memory events[2], and clear the layer cache somehow if we're in a low memory situation.
> 
> [1]: https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/dom/chrome-webidl/ChromeUtils.webidl#58-68
> [2]: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIMemory#Low_memory_notifications

That's a fair concern. However, I'm not actually sure how to hit this case without calling gBrowser.removeTab(...) from the console, given that we should have a switcher if we're hovering over a tab, even if tab warming is disabled. Windows opened from script calling window.close() on themselves, I guess?

I think I'd favor either the nsIWeakReference approach, or just removing the tab from the cache directly inside gBrowser.removeTab(...), which I think would eliminate the need for moving aTab.closing = true;. Since gBrowser does technically own the cache, I don't personally have much issue with it being able to modify the cache, but I don't know what your feelings on that are.

	Mike Conley (:mconley) (:⚙️)
	Comment 21 • 6 years ago

(In reply to Doug Thayer [:dthayer] from comment #20)
> Windows opened from script calling window.close() on themselves, I
> guess?

Yep, and add-ons - add-ons can open and close tabs arbitrarily.

> I think I'd favor either the nsIWeakReference approach, or just removing the
> tab from the cache directly inside gBrowser.removeTab(...), which I think
> would eliminate the need for moving aTab.closing = true;.

Actually, I like that - you're right, gBrowser owns the cache, so it should be in charge clearing it on removals. Let's do that.

	Mike Conley (:mconley) (:⚙️)
	Comment 23 • 6 years ago
mozreview-review

Comment on attachment 8971032 [details]
Bug 1176019 - Cache layers of background tabs

https://reviewboard.mozilla.org/r/239754/#review246190

This looks good to me! However, we probably don't want to land this until the softfreeze is over, so maybe let's just let this sit for a little while until central becomes 62.

In the meantime, we might want to add this cache to the "Perceived Performance Optimizations" section of browser/base/content/docs/tabbrowser/async-tab-switcher.rst, and describe how it works.

::: browser/modules/AsyncTabSwitcher.jsm:912
(Diff revision 2)
>      this.setTabState(tab, this.STATE_LOADING);
>      this.queueUnload(gTabWarmingUnloadDelayMs);
>    }
>  
> +  cleanUpTabAfterEviction(tab) {
> +    let browser = tab.linkedBrowser;

Let's add an assertion that the tab we're evicting isn't the requestedTab somehow.

	Pulsebot
	Comment 25 • 6 years ago

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/998791c75fbc
Cache layers of background tabs r=mconley

	Dorel Luca [:dluca]
	Comment 26 • 6 years ago

Backed out changeset 998791c75fbc (bug 1176019) for Browser-chrome failures on dom/plugins/test/mochitest/browser_bug1196539.js. CLOSED TREE 

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=177366158&repo=autoland&lineNumber=15245

742 INFO TEST-PASS | dom/plugins/test/mochitest/browser_bug1196539.js | plugin is hidden -
00:24:27     INFO -  Buffered messages finished
00:24:27    ERROR -  743 INFO TEST-UNEXPECTED-FAIL | dom/plugins/test/mochitest/browser_bug1196539.js | paint count can't be greater than zero, count was 0 -
00:24:27     INFO -  Stack trace:
00:24:27     INFO -  chrome://mochitests/content/browser/dom/plugins/test/mochitest/browser_bug1196539.js:checkPaintCount:4
00:24:27     INFO -  chrome://mochitests/content/browser/dom/plugins/test/mochitest/browser_bug1196539.js:null:115
00:24:27     INFO -  744 INFO TEST-PASS | dom/plugins/test/mochitest/browser_bug1196539.js | paint count should be within limits, count was 0 -
00:24:27     INFO -  745 INFO Leaving test bound
00:24:27     INFO -  GECKO(2796) | MEMORY STAT | vsize 2098975MB | vsizeMaxContiguous 128179972MB | residentFast 304MB | heapAllocated 121MB
00:24:27     INFO -  746 INFO TEST-OK | dom/plugins/test/mochitest/browser_bug1196539.js | took 2575ms
00:24:27     INFO -  GECKO(2796) | ++DOCSHELL 000001F67EDEC000 == 3 [pid = 1852] [id = {653a6cc3-f03a-4a10-9864-9809386b4933}]
00:24:27     INFO -  GECKO(2796) | ++DOMWINDOW == 6 (000001F67F16AA00) [pid = 1852] [serial = 6] [outer = 0000000000000000]
00:24:27     INFO -  GECKO(2796) | ++DOMWINDOW == 7 (000001F67F3C9800) [pid = 1852] [serial = 7] [outer = 000001F67F16AA00]
00:24:27     INFO -  747 INFO checking window state
00:24:27     INFO -  748 INFO TEST-START | dom/plugins/test/mochitest/browser_bug1335475.js
00:24:27     INFO -  GECKO(2796) | ++DOCSHELL 000002AD88988800 == 2 [pid = 4696] [id = {f47b42ae-8dcf-4e1c-aa18-eb0fbbf60e3c}]
00:24:27     INFO -  GECKO(2796) | ++DOMWINDOW == 4 (000002AD80F12000) [pid = 4696] [serial = 4] [outer = 0000000000000000]
00:24:27     INFO -  GECKO(2796) | ++DOMWINDOW == 5 (000002AD889E8800) [pid = 4696] [serial = 5] [outer = 000002AD80F12000]
00:24:27     INFO -  GECKO(2796) | ++DOMWINDOW == 6 (000002AD8876BC00) [pid = 4696] [serial = 6] [outer = 000002AD80F12000]
00:24:28     INFO -  GECKO(2796) | [Child 4696, Main Thread] WARNING: site security information will not be persisted: file z:/build/build/src/security/manager/ssl/nsSiteSecurityService.cpp, line 553
00:24:28     INFO -  GECKO(2796) | ++DOMWINDOW == 7 (000002AD8AB11800) [pid = 4696] [serial = 7] [outer = 000002AD80F12000]
00:24:28     INFO -  GECKO(2796) | ++DOCSHELL 000002AD8AB9E800 == 3 [pid = 4696] [id = {9827dd50-bbf7-4a45-adff-f35c492ab258}]
00:24:28     INFO -  GECKO(2796) | ++DOMWINDOW == 8 (000002AD8A0F4600) [pid = 4696] [serial = 8] [outer = 0000000000000000]
00:24:28     INFO -  GECKO(2796) | For application/x-test found plugin nptest.dll
00:24:28     INFO -  GECKO(2796) | ++DOMWINDOW == 9 (000002AD8B00B400) [pid = 4696] [serial = 9] [outer = 000002AD8A0F4600]
00:24:28     INFO -  GECKO(2796) | ++DOMWINDOW == 10 (000002AD8B00FC00) [pid = 4696] [serial = 10] [outer = 000002AD80F12000]
00:24:28     INFO -  GECKO(2796) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to c:\users\task_1525737823\appdata\local\temp\tmp2bwvkv.mozrunner\runtests_leaks_tab_pid3652.log
00:24:28     INFO -  GECKO(2796) | [Child 3652, Main Thread] WARNING: No CID found when attempting to map contract ID: file z:/build/build/src/xpcom/components/nsComponentManager.cpp, line 505
00:24:28     INFO -  GECKO(2796) | Unable to read VR Path Registry from Z:\task_1525737823\AppData\Local\openvr\openvrpaths.vrpath
00:24:28     INFO -  GECKO(2796) | [Child 3652, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file z:/build/build/src/xpcom/base/nsSystemInfo.cpp, line 119
00:24:28     INFO -  GECKO(2796) | ++DOCSHELL 000002054FD47000 == 1 [pid = 3652] [id = {0cf2978e-6e52-404a-ab9e-7be532dd0370}]
00:24:28     INFO -  GECKO(2796) | ++DOMWINDOW == 1 (000002054FD11A00) [pid = 3652] [serial = 1] [outer = 0000000000000000]
00:24:28     INFO -  GECKO(2796) | ++DOMWINDOW == 2 (0000020555399C00) [pid = 3652] [serial = 2] [outer = 000002054FD11A00]
00:24:28     INFO -  GECKO(2796) | ++DOMWINDOW == 3 (0000020557559C00) [pid = 3652] [serial = 3] [outer = 000002054FD11A00]
00:24:28     INFO -  GECKO(2796) | ++DOCSHELL 00000205577BA800 == 2 [pid = 3652] [id = {815f6114-b1fb-4f20-831e-856eada27467}]
00:24:28     INFO -  GECKO(2796) | ++DOMWINDOW == 4 (0000020557571A00) [pid = 3652] [serial = 4] [outer = 0000000000000000]
00:24:28     INFO -  GECKO(2796) | ++DOMWINDOW == 5 (0000020557C10400) [pid = 3652] [serial = 5] [outer = 0000020557571A00]
00:24:28     INFO -  GECKO(2796) | [Parent 2796, Gecko_IOThread] WARNING: pipe error: 109: file z:/build/build/src/ipc/chromium/src/chrome/common/ipc_channel_win.cc, line 346
00:24:28     INFO -  GECKO(2796) | MEMORY STAT | vsize 2098983MB | vsizeMaxContiguous 128179972MB | residentFast 314MB | heapAllocated 124MB
00:24:28     INFO -  749 INFO TEST-OK | dom/plugins/test/mochitest/browser_bug1335475.js | took 1262ms
00:24:28     INFO -  GECKO(2796) | ++DOCSHELL 00000223A215A800 == 3 [pid = 8100] [id = {e2206ac8-a156-4621-bafa-48cc0b3c4a7d}]
00:24:28     INFO -  GECKO(2796) | ++DOMWINDOW == 6 (00000223A2178400) [pid = 8100] [serial = 6] [outer = 0000000000000000]
00:24:29     INFO -  GECKO(2796) | ++DOMWINDOW == 7 (00000223A3FE6C00) [pid = 8100] [serial = 7] [outer = 00000223A2178400]
00:24:29     INFO -  750 INFO checking window state

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=998791c75fbcf1ab689ae822be0a2c33e98ff1f7

Backout:
https://hg.mozilla.org/integration/autoland/rev/bc29be44bbab41270339ca771d7f8d22a4be4484

	Mike Conley (:mconley) (:⚙️)
	Comment 30 • 6 years ago
mozreview-review

Comment on attachment 8974194 [details]
Bug 1176019 - Fix browser_bug1196539.js painting check

https://reviewboard.mozilla.org/r/242484/#review248382

Seems like a fine band-aid here.

	Mike Conley (:mconley) (:⚙️)
	Comment 31 • 6 years ago
mozreview-review

Comment on attachment 8974195 [details]
Bug 1176019 - Fix browser_tabswitchbetweenplugins.js

https://reviewboard.mozilla.org/r/242486/#review248632

Clearing r?, mostly because I think we should ask jimm about what to do here. Or maybe we can just wait for MozAfterPaint. At the very least, I don't think I'm the right person to make the final call on how to fix this test.

::: dom/plugins/test/mochitest/browser_tabswitchbetweenplugins.js:4
(Diff revision 1)
>  var gTestRoot = getRootDirectory(gTestPath).replace("chrome://mochitests/content/", "http://127.0.0.1:8888/");
>  
> +function waitForPluginVisibility(browser, shouldBeVisible, errorMessage) {
> +  return BrowserTestUtils.waitForCondition(async () => {

Something about this seems off... so, I guess we're keeping the layers around, presumably, for these test tabs...

At some point, I guess the compositor receives some kind of signal to update "plugin windows" here: https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/gfx/layers/composite/AsyncCompositionManager.cpp#147-152, and that, I guess, sends a message to the parent process(?) to update plugin visibility over here:

https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/gfx/layers/ipc/CompositorBridgeChild.cpp#464

which, I guess sets visibility to false here for the plugins that shouldn't be visible:

https://searchfox.org/mozilla-central/rev/5ff2d7683078c96e4b11b8a13674daded935aa44/widget/nsBaseWidget.cpp#2350

So I guess what happens here is that we complete the tab switch to the new tab, and the old ones layers are still on the compositor because it's cached, and then AsyncTabSwitcher fires the TabSwitchDone event, which causes our test here to proceed.

And I guess maybe there's a race now where the ContentTask message to the content process to ask about the visible state of the plugin, and the message from the compositor to the parent (? or content process? or plugin process? This part is really fuzzy for me...) to update visibility state of the plugin...

Instead of waiting for the arbitrary 200ms, can we instead wait for a message from the compositor that we know must fire _after_ the update-the-plugin-visibility message goes out? MozAfterPaint maybe?

If that turns out to be a quagmire, this band-aid _might_ be okay, but I'm a bit less confident I'm the right person to make that call for this particular test. jimm might be the better person to ask to sign-off on it.

My only fear, ultimately, is that we're introducing a case where plugins from background tabs that we're switching from might continue to be displayed overtop of the foreground tab - even if it's just for a few milliseconds.

	Mike Conley (:mconley) (:⚙️)
	Comment 33 • 6 years ago
mozreview-review

Comment on attachment 8974195 [details]
Bug 1176019 - Fix browser_tabswitchbetweenplugins.js

https://reviewboard.mozilla.org/r/242486/#review248998

I think this is probably okay, but is it possible to just wait for the MozAfterPaint? If not, let's go your route.

::: dom/plugins/test/mochitest/browser_tabswitchbetweenplugins.js:24
(Diff revisions 1 - 2)
> +        // still don't have the correct visibility, that's likely a
> +        // problem.
> +        reject();
> +      }
> +    };
> +    window.addEventListener("MozAfterPaint", listener);

This seems okay - but can we just wait for the MozAfterPaint rather than calling listener() directly? I feel like that would simplify things a bit.

OH, and just a thing to note about MozAfterPaint - see https://groups.google.com/forum/#!searchin/mozilla.dev.platform/MozAfterPaint%7Csort:date/mozilla.dev.platform/pCLwWdYc_GY/j9A-vWm3AgAJ for details if you're noticing MozAfterPaints where the visibility is still not what you expect.

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 34 • 6 years ago
mozreview-review-reply

Comment on attachment 8974195 [details]
Bug 1176019 - Fix browser_tabswitchbetweenplugins.js

https://reviewboard.mozilla.org/r/242486/#review248998

> This seems okay - but can we just wait for the MozAfterPaint rather than calling listener() directly? I feel like that would simplify things a bit.
> 
> OH, and just a thing to note about MozAfterPaint - see https://groups.google.com/forum/#!searchin/mozilla.dev.platform/MozAfterPaint%7Csort:date/mozilla.dev.platform/pCLwWdYc_GY/j9A-vWm3AgAJ for details if you're noticing MozAfterPaints where the visibility is still not what you expect.

We call this function in succession for each tab, so there's no guarantee that there's actually going to be a MozAfterPaint coming each time. Additionally, if the tab we're switching to isn't in the cache, then we already waited for the MozAfterPaint in waiting for the tab switch, so we might not get it. Surprisingly, not calling listener() directly does work, at least locally, and I'm not sure why we're getting enough MozAfterPaint's for it to work. In either case, checking the visibility once before waiting for a MozAfterPaint just seemed like a more robust solution. Let me know if you disagree or if I'm missing something though.

	Mike Conley (:mconley) (:⚙️)
	Comment 35 • 6 years ago
mozreview-review-reply

Comment on attachment 8974195 [details]
Bug 1176019 - Fix browser_tabswitchbetweenplugins.js

https://reviewboard.mozilla.org/r/242486/#review248998

> We call this function in succession for each tab, so there's no guarantee that there's actually going to be a MozAfterPaint coming each time. Additionally, if the tab we're switching to isn't in the cache, then we already waited for the MozAfterPaint in waiting for the tab switch, so we might not get it. Surprisingly, not calling listener() directly does work, at least locally, and I'm not sure why we're getting enough MozAfterPaint's for it to work. In either case, checking the visibility once before waiting for a MozAfterPaint just seemed like a more robust solution. Let me know if you disagree or if I'm missing something though.

Nah, let's not worry about it for this test. Let's land this puppy!

	Pulsebot
	Comment 36 • 6 years ago

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e7dbe6b5122
Cache layers of background tabs r=mconley
https://hg.mozilla.org/integration/autoland/rev/6c591b484a11
Fix browser_bug1196539.js painting check r=mconley
https://hg.mozilla.org/integration/autoland/rev/31b295f557db
Fix browser_tabswitchbetweenplugins.js r=mconley

	Dorel Luca [:dluca]
	Comment 37 • 6 years ago

Backed out 3 changesets (bug 1176019) for Browser-chrome failures on dom/plugins/test/mochitest/browser_tabswitchbetweenplugins.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=177955039&repo=autoland&lineNumber=16120

653 INFO TEST-PASS | dom/plugins/test/mochitest/browser_tabswitchbetweenplugins.js | plugin2 is selected -
22:32:58     INFO -  Buffered messages finished
22:32:58    ERROR -  654 INFO TEST-UNEXPECTED-FAIL | dom/plugins/test/mochitest/browser_tabswitchbetweenplugins.js | Uncaught exception -
22:32:58     INFO -  655 INFO Leaving test bound
22:32:58     INFO -  GECKO(3068) | MEMORY STAT | vsize 2099088MB | vsizeMaxContiguous 129892985MB | residentFast 340MB | heapAllocated 110MB
22:32:58     INFO -  656 INFO TEST-OK | dom/plugins/test/mochitest/browser_tabswitchbetweenplugins.js | took 1389ms
22:32:58     INFO -  Not taking screenshot here: see the one that was previously logged
22:32:58    ERROR -  657 INFO TEST-UNEXPECTED-FAIL | dom/plugins/test/mochitest/browser_tabswitchbetweenplugins.js | Found an unexpected tab at the end of test run: http://127.0.0.1:8888/browser/dom/plugins/test/mochitest/plugin_test.html -
22:32:58     INFO -  GECKO(3068) | [Child 1048, Main Thread] ###!!! ASSERTION: does it make sense to Show()/Hide() a disabled widget?: 'mEnabled', file z:/build/build/src/widget/PuppetWidget.cpp, line 208
22:32:58     INFO -  GECKO(3068) | #01: nsPluginInstanceOwner::UpdateDocumentActiveState(bool) [dom/plugins/base/nsPluginInstanceOwner.cpp:3202]
22:32:58     INFO -  GECKO(3068) | #02: nsPluginFrame::SetIsDocumentActive(bool) [layout/generic/nsPluginFrame.cpp:1739]
22:32:58     INFO -  GECKO(3068) | #03: static void SetPluginIsActive(class nsISupports *, void *) [layout/base/PresShell.cpp:10432]
22:32:58     INFO -  GECKO(3068) | #04: nsIDocument::EnumerateActivityObservers(void (*)(nsISupports *,void *),void *) [dom/base/nsDocument.cpp:9523]
22:32:58     INFO -  GECKO(3068) | #05: mozilla::PresShell::SetIsActive(bool) [layout/base/PresShell.cpp:10455]
22:32:58     INFO -  GECKO(3068) | #06: nsDocShell::SetIsActive(bool) [docshell/base/nsDocShell.cpp:5760]
22:32:58     INFO -  GECKO(3068) | #07: mozilla::dom::TabChild::InternalSetDocShellIsActive(bool) [dom/ipc/TabChild.cpp:2553]
22:32:58     INFO -  GECKO(3068) | #08: mozilla::dom::TabChild::RecvSetDocShellIsActive(bool const &) [dom/ipc/TabChild.cpp:2568]
22:32:58     INFO -  GECKO(3068) | #09: mozilla::dom::PBrowserChild::OnMessageReceived(IPC::Message const &) [s3:gecko-generated-sources:9a88dd75b3dc75e8154ddef2f047fece42b19a07139073a5a0a6b3d489da84ac4df07180672ca794085eca4bad552c419650a45ea3ee19f4e65ec4671e653e96/ipc/ipdl/PBrowserChild.cpp::4670]
22:32:58     INFO -  GECKO(3068) | #10: mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const &) [s3:gecko-generated-sources:beb29ed80331f819f946475df574498a584084caef02cfa8c6078589221a4714017a415ab8751410f0f9629339ac0242ba76aa13b5aa5ca6e1c94d7d102bc024/ipc/ipdl/PContentChild.cpp::5316]
22:32:58     INFO -  GECKO(3068) | #11: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const &) [ipc/glue/MessageChannel.cpp:2136]
22:32:58     INFO -  GECKO(3068) | #12: mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message &&) [ipc/glue/MessageChannel.cpp:2067]
22:32:58     INFO -  GECKO(3068) | #13: mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask &) [ipc/glue/MessageChannel.cpp:1912]
22:32:58     INFO -  GECKO(3068) | #14: mozilla::ipc::MessageChannel::MessageTask::Run() [ipc/glue/MessageChannel.cpp:1945]
22:32:58     INFO -  GECKO(3068) | #15: mozilla::SchedulerGroup::Runnable::Run() [xpcom/threads/SchedulerGroup.cpp:341]
22:32:58     INFO -  GECKO(3068) | #16: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1093]
22:32:58     INFO -  GECKO(3068) | #17: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/threads/nsThreadUtils.cpp:519]
22:32:58     INFO -  GECKO(3068) | #18: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:125]
22:32:58     INFO -  GECKO(3068) | #19: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:301]
22:32:58     INFO -  GECKO(3068) | #20: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:320]
22:32:58     INFO -  GECKO(3068) | #21: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:300]
22:32:58     INFO -  GECKO(3068) | #22: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:159]
22:32:58     INFO -  GECKO(3068) | #23: nsAppShell::Run() [widget/windows/nsAppShell.cpp:403]
22:32:58     INFO -  GECKO(3068) | #24: XRE_RunAppShell() [toolkit/xre/nsEmbedFunctions.cpp:893]
22:32:58     INFO -  GECKO(3068) | #25: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:269]
22:32:58     INFO -  GECKO(3068) | #26: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:320]
22:32:58     INFO -  GECKO(3068) | #27: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:300]
22:32:58     INFO -  GECKO(3068) | #28: XRE_InitChildProcess(int,char * * const,XREChildData const *) [toolkit/xre/nsEmbedFunctions.cpp:723]
22:32:58     INFO -  GECKO(3068) | #29: content_process_main(mozilla::Bootstrap *,int,char * * const) [ipc/contentproc/plugin-container.cpp:51]
22:32:58     INFO -  GECKO(3068) | #30: NS_internal_main(int,char * *,char * *) [browser/app/nsBrowserApp.cpp:285]
22:32:58     INFO -  GECKO(3068) | #31: wmain [toolkit/xre/nsWindowsWMain.cpp:143]
22:32:58     INFO -  GECKO(3068) | #32: static int __scrt_common_main_seh() [f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:283]
22:32:58     INFO -  GECKO(3068) | #33: KERNEL32.DLL + 0x12774
22:32:58     INFO -  GECKO(3068) | #34: ntdll.dll + 0x70d61
22:32:58     INFO -  Not taking screenshot here: see the one that was previously logged

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=31b295f557db579744feb07900a5aa5e145452b0

Backout:
https://hg.mozilla.org/integration/autoland/rev/4e08519eb8f0726820d67ac458d289d9dc503006

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 39 • 6 years ago

Applied the lastTransactionId changes mentioned in the google doc in comment 33. The try looks good, at least for 9 runs of the job containing that test. Fingers crossed.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c9a51dfc5a61fffb21a95f13f6e96f8b7d09ba1&selectedJob=177991418

	Pulsebot
	Comment 40 • 6 years ago

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d0dfc54128e
Cache layers of background tabs r=mconley
https://hg.mozilla.org/integration/autoland/rev/9c6b0eb062ca
Fix browser_bug1196539.js painting check r=mconley
https://hg.mozilla.org/integration/autoland/rev/2dd8b719d645
Fix browser_tabswitchbetweenplugins.js r=mconley

	Eliza Balazs [:ebalazs_]
	Comment 41 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2d0dfc54128e
https://hg.mozilla.org/mozilla-central/rev/9c6b0eb062ca
https://hg.mozilla.org/mozilla-central/rev/2dd8b719d645

	Eliza Balazs [:ebalazs_]
	Comment 42 • 6 years ago

Backed out 3 changesets (bug 1176019) for frequently failing /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Maybe.h
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=178024286&repo=mozilla-central&lineNumber=17330
Backout: https://hg.mozilla.org/mozilla-central/rev/4303d49c53931385892231969e40048f096b4d4c

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 43 • 6 years ago

Hiro, I'm wondering if you can help me understand this any better? My patch seems to be crashing unwrapping the Maybe on this line: https://searchfox.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#722. The test seems to fail when switching between tabs very quickly (can't reproduce it locally). My patch does upset the usual ordering of a tab switch because with it we hold onto recently used tabs' layers, resulting in instant tab switches. Do you have any ideas why this change might result in a crash there?

	Hiroyuki Ikezoe (:hiro)
	Comment 44 • 6 years ago

Doug, I am sorry for the bothering you about the assertion.
I think your changes un-wallpapered an underlying issue that causes the assertion.

(In reply to Doug Thayer [:dthayer] from comment #43)
> Hiro, I'm wondering if you can help me understand this any better? My patch
> seems to be crashing unwrapping the Maybe on this line:
> https://searchfox.org/mozilla-central/source/gfx/layers/composite/
> AsyncCompositionManager.cpp#722. The test seems to fail when switching
> between tabs very quickly (can't reproduce it locally).

This information is pretty helpful to understand what's going on there.
I will fix the underlying issue in bug 1461023.  But for now, I am going to disable the assertion in bug 1461023.

Thanks!

	Hiroyuki Ikezoe (:hiro)
	Comment 45 • 6 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #44)
> 
> This information is pretty helpful to understand what's going on there.
> I will fix the underlying issue in bug 1461023.  But for now, I am going to
> disable the assertion in bug 1461023.

Wrong bug number..

I meant, I will fix the issue in bug 1459775.

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 46 • 6 years ago

(In reply to Hiroyuki Ikezoe (:hiro) from comment #44)
> This information is pretty helpful to understand what's going on there.
> I will fix the underlying issue in bug 1461023.  But for now, I am going to
> disable the assertion in bug 1461023.
> 
> Thanks!

Thank you for the quick response! :)

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 47 • 6 years ago

mconley, what do you think we should do regarding Bug 1460905? The way I see it we have two strategies we can mix/match:

1. Force the paint when we switch tabs, so that the stale content will only stay around for as long as the tab switcher would have stayed around previously.
2. Try to invalidate the cache when the layers have changed.

#1 alone means we could have flashes of stale content which might differ greatly from what _should_ be displayed, and #2 alone means we might be invalidating the cache for trivial differences, and best I can tell it would be rather complicated to implement. I'm leaning toward #1, with a special case where we invalidate the cache for a background tab at some point during page load.

	Mike Conley (:mconley) (:⚙️)
	Comment 48 • 6 years ago

(In reply to Doug Thayer [:dthayer] from comment #47)
> #1 alone means we could have flashes of stale content which might differ
> greatly from what _should_ be displayed, and #2 alone means we might be
> invalidating the cache for trivial differences, and best I can tell it would
> be rather complicated to implement. I'm leaning toward #1, with a special
> case where we invalidate the cache for a background tab at some point during
> page load.

I'd be interested in knowing what Chromium strategy is here. Can you dig in and see what they do to avoid the flashes of stale content? (Since I can't seem to reproduce the bug in Chromium)

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 49 • 6 years ago

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up on needinfos / reviews) from comment #48)
> I'd be interested in knowing what Chromium strategy is here. Can you dig in
> and see what they do to avoid the flashes of stale content? (Since I can't
> seem to reproduce the bug in Chromium)

It looks similar to what I proposed, as far as I can tell, probably with more special cases though. If you stress your CPU to 100% to slow Chrome down, and follow the STR in Bug 1460905, you'll see blank content and then it will paint the new page. However if you create a page that just changes a div's color every 5 seconds, and switch away from it for more than 5 seconds, when you come back you'll see a flash of the color that you left it on.

	Mike Conley (:mconley) (:⚙️)
	Comment 50 • 6 years ago

Alright - let's go with #1 then, keeping in mind / structuring it so that we might want to add some heuristics or edge-case handlers to invalidate the cache to avoid weird flicker.

Thanks!

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 55 • 6 years ago

Went down a rabbit hole yesterday trying to figure out why Bug 1460905 didn't seem to be affecting tab warming. Turns out it does affect tab warming - it's just intermittent (and naturally you have to tweak the prefs to make it convenient to reproduce). I was trying to sort out if I needed to special case tab warming for this, and if I did, why, since a warmed tab and a cached tab should be in the same state.

	Mike Conley (:mconley) (:⚙️)
	Comment 58 • 6 years ago
mozreview-review

Comment on attachment 8975841 [details]
Bug 1176019 - Force a paint when switching to a loaded tab

https://reviewboard.mozilla.org/r/244028/#review250402

Thanks! Structurally, this seems sound. I don't think it needs re-review, but certainly the naming needs to be worked out before this lands.

::: dom/ipc/PBrowser.ipdl:765
(Diff revision 1)
>       * clears the layers from the compositor.
>       *
>       * @param aEnabled
>       *        True if the child should render and upload layers, false if the
>       *        child should clear layers.
> +     * @param aForce

Maybe aForceRepaint? Naming is hard.

::: dom/ipc/PProcessHangMonitor.ipdl:44
(Diff revision 1)
>    async TerminateScript(bool aTerminateGlobal);
>  
>    async BeginStartingDebugger();
>    async EndStartingDebugger();
>  
> -  async ForcePaint(TabId tabId, uint64_t aLayerObserverEpoch);
> +  async ForcePaint(TabId tabId, bool force, uint64_t aLayerObserverEpoch);

We could rename ForcePaint to either of your suggestions, or we could maybe rename the argument to "allowRepaint" or something.

I do, however, think it might be better to rename ForcePaint. PaintWithInterrupt might be made clearer as PaintWithJSInterrupt...

or maybe PaintWhileInterruptingJS.

I feel like maybe PaintWhileInterruptingJS is clearest. How do you feel about that?

::: dom/ipc/ProcessHangMonitor.cpp:302
(Diff revision 1)
>     mTerminateScript(false),
>     mTerminateGlobal(false),
>     mStartDebugger(false),
>     mFinishedStartingDebugger(false),
>     mForcePaint(false),
> +   mForcePaintForce(false),

Yep, probably should rename one of these things before any of this lands.

::: dom/ipc/TabChild.cpp:2541
(Diff revision 1)
>      mPendingDocShellReceivedMessage = false;
>      InternalSetDocShellIsActive(mPendingDocShellIsActive);
>    }
>    if (!mPendingDocShellBlockers && mPendingRenderLayersReceivedMessage) {
>      mPendingRenderLayersReceivedMessage = false;
> -    RecvRenderLayers(mPendingRenderLayers, mPendingLayerObserverEpoch);
> +    RecvRenderLayers(mPendingRenderLayers, false, mPendingLayerObserverEpoch);

Same note as above, re: boolean arguments. It's a personal preference, and I'm willing to take pushback. It's not, like, Mozilla policy or anything.

::: dom/ipc/TabParent.cpp:2923
(Diff revision 1)
>      return NS_OK;
>    }
>  
>    mRenderLayers = aEnabled;
>  
> -  // Increment the epoch so that layer tree updates from previous
> +  SetRenderLayersInternal(aEnabled, false);

For boolean arguments like this, I sometimes find it's clearer to do something like:

```
SetRenderLayersInternal(aEnabled, false /* aForce */);
```

Up to you, but I think it'd make it easier to read and understand.

::: dom/ipc/TabParent.cpp:2944
(Diff revision 1)
>  }
>  
>  NS_IMETHODIMP
> +TabParent::ForcePaint()
> +{
> +  SetRenderLayersInternal(true, true);

Same note as above, this might be easier to read as:

```
SetRenderLayersInternal(true /* aEnabled */,
                        true /* aForce */);
```

	Mike Conley (:mconley) (:⚙️)
	Comment 59 • 6 years ago
mozreview-review

Comment on attachment 8975841 [details]
Bug 1176019 - Force a paint when switching to a loaded tab

https://reviewboard.mozilla.org/r/244028/#review250412

::: browser/modules/AsyncTabSwitcher.jsm:983
(Diff revision 1)
>  
>      this.requestedTab = tab;
>      if (tabState == this.STATE_LOADED) {
>        this.maybeVisibleTabs.clear();
> +      if (tab.linkedBrowser.isRemoteBrowser) {
> +        tab.linkedBrowser.forcePaint();

I guess we still risk showing a stale frame if we flip the deck before the compositor receives the updated layers?

	Mike Conley (:mconley) (:⚙️)
	Comment 60 • 6 years ago
mozreview-review

Comment on attachment 8975952 [details]
Bug 1176019 - Clear cached layers on location change

https://reviewboard.mozilla.org/r/244116/#review250414

Makes sense - thanks!

	Mike Conley (:mconley) (:⚙️)
	Comment 61 • 6 years ago
mozreview-review

Comment on attachment 8975953 [details]
Bug 1176019 - Don't let cache interfere with tab warming

https://reviewboard.mozilla.org/r/244118/#review250416

You're right - this does make more sense, and I'm sorry I didn't catch it the first time around! Thanks.

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 69 • 6 years ago
mozreview-review-reply

Comment on attachment 8975841 [details]
Bug 1176019 - Force a paint when switching to a loaded tab

https://reviewboard.mozilla.org/r/244028/#review250402

> We could rename ForcePaint to either of your suggestions, or we could maybe rename the argument to "allowRepaint" or something.
> 
> I do, however, think it might be better to rename ForcePaint. PaintWithInterrupt might be made clearer as PaintWithJSInterrupt...
> 
> or maybe PaintWhileInterruptingJS.
> 
> I feel like maybe PaintWhileInterruptingJS is clearest. How do you feel about that?

PaintWhileInterruptingJS works for me!

	Pulsebot
	Comment 73 • 6 years ago

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9850536880ee
Cache layers of background tabs r=mconley
https://hg.mozilla.org/integration/autoland/rev/8ff26293b898
Fix browser_bug1196539.js painting check r=mconley
https://hg.mozilla.org/integration/autoland/rev/dd85a2be4527
Fix browser_tabswitchbetweenplugins.js r=mconley
https://hg.mozilla.org/integration/autoland/rev/27f6f789b194
Force a paint when switching to a loaded tab r=mconley
https://hg.mozilla.org/integration/autoland/rev/94bfb6800a01
Clear cached layers on location change r=mconley
https://hg.mozilla.org/integration/autoland/rev/9c10d9a71983
Don't let cache interfere with tab warming r=mconley

	Narcis Beleuzu [:NarcisB]
	Comment 74 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/9850536880ee
https://hg.mozilla.org/mozilla-central/rev/8ff26293b898
https://hg.mozilla.org/mozilla-central/rev/dd85a2be4527
https://hg.mozilla.org/mozilla-central/rev/27f6f789b194
https://hg.mozilla.org/mozilla-central/rev/94bfb6800a01
https://hg.mozilla.org/mozilla-central/rev/9c10d9a71983

	Doug Thayer [:dthayer] (he/him) Assignee
	Comment 75 • 6 years ago

mconley, trying to decide if we should leave this on in Nightly given bug 1464244, bug 1464712, and bug 1463397. I would say we've gotten the information we need from the spinner graphs at https://mikeconley.github.io/bug1310250/. I'm inclined to disable this, discuss our options regarding memory usage and whether the observed benefit is worth the trade, and look into causes of the issues in the linked bugs. Thoughts?

	Mike Conley (:mconley) (:⚙️)
	Comment 76 • 6 years ago

Yeah, that sounds like a fine idea. Let's do it.