1876604 - "document.visibilityState" of the formerly active tab is not set to "hidden" after creating or switching between tabs

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I noticed that while investigating a new Puppeteer unit test for context activation. Here the document.visibilityState is used to check if a document is visible or hidden. And that check fails because it always returns visible.

This can be easily reproduced in the WebDriver BiDi test client by running the following code in the DevTool's console:

async function getVisibilityState(context) {
  const res = await sendCommand("script.evaluate", {
    expression: "document.visibilityState;",
    target: context,
    awaitPromise: false,
  });
  return res.result.result;
}

function sleep(time = 1000) {
  return new Promise(resolve => window.setTimeout(resolve, time));
}

(async function () {
  await sendCommand("session.new", { capabilities: {} });
  let res = await sendCommand("browsingContext.getTree", {});
  let context = res.result.contexts[0].context;

  const delay = 0;  // fails
  // const delay = 325; // passes

  await sendCommand("browsingContext.navigate", {
    context,
    url: "http://example.org",
    wait: "complete",
  });

  console.log("before opening the tab");
  console.log(await getVisibilityState({ context }));

  let tab1 = await sendCommand("browsingContext.create", {
    type: "tab"
  });

  console.log("after opening the tab");
  console.log(await getVisibilityState({ context }));
  console.log(await getVisibilityState({
    context: tab1.result.context
  }));
  await sleep(delay);

  console.log("after sleep");
  console.log(await getVisibilityState({ context }));
  console.log(await getVisibilityState({
    context: tab1.result.context
  }));
  await sleep(delay);

  await sendCommand("browsingContext.navigate", {
    context: tab1.result.context,
    url: "http://mozilla.org",
    wait: "complete",
  });

  console.log("after navigate in tab");
  console.log(await getVisibilityState({ context }));
  console.log(await getVisibilityState({
    context: tab1.result.context
  }));
  await sleep(delay);

  await sendCommand("browsingContext.activate", { context });

  console.log("after switching the tab");
  console.log(await getVisibilityState({ context }));
  console.log(await getVisibilityState({
    context: tab1.result.context
  }));
  await sleep(delay);

  console.log("after sleep");
  console.log(await getVisibilityState({ context }));
  console.log(await getVisibilityState({
    context: tab1.result.context
  }));

  await sendCommand("browsingContext.close", {
    context: tab1.result.context
  });
})()

As it can be seen the visibility state shows visible for the original tab and the newly opened one. Adding an additional delay makes it work. So it means that we have some race condition here, or maybe we have to explicitly wait until the visibilityState of the document in the content process has been updated?

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As it looks like there is a BrowsingContext.isActive flag that we might be able to use here. That would be great so that we won't have to do any IPC call.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

After some investigation I can see that the browsing context's isActive flag of the previously selected tab is set to false with a delay of around 300ms! And the same also applies to the visibilityState of the document as it can be seen in this Firefox profile:

https://share.firefox.dev/3UeoHM3

This delay doesn't seem to be related to mozilla.org. Other sites like example.com take the same amount of time.

Olli, is it visible from this profile why it takes that long until the document is marked as hidden?

Comment 3

[Florian Quèze [:florian]]

The 300ms delay comes from https://searchfox.org/mozilla-central/rev/cee2c396081d950f9e3401113fb179999e404ab8/browser/modules/AsyncTabSwitcher.sys.mjs#85

AsyncTabSwitcher keeps tabs in the visible state for 300ms after they've been switched away from so that switching back and forth between tabs is fast and doesn't involve expensive graphics operations.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Came here to comment about the async tab switcher and Florian was faster :)

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This is problematic for us when switching tabs with WebDriver. Chrome doesn't have this behavior and sets the value immediately because a test like that passes there. It means that tests that rely on the visibilityState immediately updated will fail or Firefox. I might want to check Safari as well and when the state is updated there. Having such differences between browsers makes it harder to write cross-browser tests.

How is it for minimized windows? Is there the delay as well? document.hidden has the same behavior.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

FYI I actually cannot test with Safari because it doesn't implement WebDriver BiDi yet and with WebDriver classic this scenario cannot be tested because I would have to switch to the appropriate browsing context first before evaluating document.visibilityState.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I had a look at the HTML spec but the section update the visibility state doesn't contain anything about timing when it should actually be updated.

Would it be fine when this timeout value could be made configurable via a preference similar to other features? That way we could set its value to 0 for general automation scenarios via the Remote Protocols (WebDriver, CDP), but also have the option to let specific tests for eg. performance reset the value to explicitly allow testing this scenario. That would make our behavior cross-browser compatible for WebDriver, and we would not have to delay any response when switching tabs by 300ms!

Note that the current scenario is less than ideal for WebDriver clients given that Firefox would require special attention by anyone trying to use this property for pages in background tabs.

Comment 8

[Florian Quèze [:florian]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #7)

Would it be fine when this timeout value could be made configurable via a preference similar to other features?

I'm concerned by the added maintenance burden of having tab switching be sometimes synchronous and sometimes async.

Alternatively, would it be possible to let the front-end mark the docshell as inactive immediately, and offer another API to indicate that layers should be preserved for 300ms?

Marking the docshell inactive immediately would also have the advantage that timers would be throttled immediately. Currently they keep firing at a fast pace for 300ms (example profile: https://share.firefox.dev/499xUcB).

Ccing mconley who I think initially added this 300ms delay in bug 1009628.

Lots of existing browser-chrome tests depend on this 300ms timer. I tried to remove that dependency in bug 1702637 and gave up as the number of hard to debug failures was massive. Gijs tried too at some point, and stopped too, probably also due to the massive amount of work to fix the failures.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

I don't have a strong opinion on the timer itself. I'm also not the right person to make that call.

Alternatively, would it be possible to let the front-end mark the docshell as inactive immediately, and offer another API to indicate that layers should be preserved for 300ms?

That seems like it should already be doable with the existing APIs.

Comment 10

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

That seems like it should already be doable with the existing APIs.

I agree - renderLayers is supposed to be the part that controls whether or not the layers are preserved. isActive is just for DocShell-activeness (which also, as I recall, will clear layers if set to false - unless preserveLayers(true) has been set on the browser).

This is all rather a mess, I'm afraid. :/

At the risk of complicating an already complicated piece of code, perhaps we can adjust the AsyncTabSwitcher to only ever have the foreground tab have isActive set to true, and otherwise any tab that is in STATE_LOADED should have preserveLayers(true), renderLayers(true), isActive(false).

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that this bug turns out to be one for AsyncTabSwitcher now lets correct its component. I'll file a new bug for WebDriver BiDi that I can use to add a further testcase to verify that this feature works.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #10)

At the risk of complicating an already complicated piece of code, perhaps we can adjust the AsyncTabSwitcher to only ever have the foreground tab have isActive set to true, and otherwise any tab that is in STATE_LOADED should have preserveLayers(true), renderLayers(true), isActive(false).

I guess that would break tab warming etc for non-remote tabs, but perhaps that's fine nowadays.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Actually not tab warming, but it would regress tabswitch delay probably. Still perhaps not a big deal.

Comment 14

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

Hey whimboo,

If not immediately obvious from the other comments in this bug, AsyncTabSwitcher is kind of a gnarly sensitive mess. We're loathe to add any new wrinkles or complications to it, if we can avoid it.

You mentioned in comment 0, "maybe we have to explicitly wait until the visibilityState of the document in the content process has been updated?"

Considering that this is just for tests being written, is this something you might consider doing, and waiting for the visibilitychange event instead of the timer?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Yes it's for testing and in most cases tests might not care about a tab that just got put into background. But we sadly cannot predict what users actually have in mind. They could indeed switch often between tabs and rely on the visbilityState. Given that Chrome updates it immediately it will feel to them like a bug in Firefox.

Note that we cannot await for the visibilitychange event given that it has the same 300ms delay as any other event that I tried. Doing that for each call of browsingContext.activate would add a huge performance overhead to tests which is clearly not acceptable. See this except from the profile as given in comment 0: https://share.firefox.dev/4buWdUc.

If we are not going / to be able to put a fix in place we would have to accept it.

So what about a preference as I've mentioned above? Then the 300ms could be adjusted and tests can set it to 0 with the caveat that after faster switching we have to re-render. But that might be acceptable?

Comment 16

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

Attachment: Bug 1876604 - Add support for browser.tabs.remote.unloadDelayMs pref. r?emilio!

This hidden preference controls the displaylist unload timeout when tabs
are backgrounded. This is mainly for use by Puppeteer / WebDriver clients
to avoid long timeouts waiting for visibilitystate to change in
backgrounded tabs.

Comment 17

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

Hey whimboo, does this patch (plus setting browser.tabs.remote.unloadDelayMs to 0) work for you?

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #17)

Hey whimboo, does this patch (plus setting browser.tabs.remote.unloadDelayMs to 0) work for you?

Thanks Mike for the quick patch! I wasn't able to test is completely yet but I can say for sure that I can set the preference and as such control the behavior of unloading the former tab. As such and the fact that it is already reviewed I would say lets get it landed.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54623e43b90d
Add support for browser.tabs.remote.unloadDelayMs pref. r=emilio

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/54623e43b90d