1635914 - browser.docShellIsActive is false by default

Status: RESOLVED FIXED

(GeckoView :: General, defect, P2)

Description

[[ex-Mozilla] Agi Sferro | :agi]

Looks like docShellIsActive is false by default which breaks our expectation that sessions are active by default. We should try to fix this in docShell directly if we can.

Comment 1

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1635914 - Add workaround for docShellIsActive.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1122e0d6545e
Add workaround for docShellIsActive. r=snorp

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1122e0d6545e

Comment 6

[Nika Layzell [:nika] (ni? for response)]

This is actually somewhat complicated, and a bit weird and buggy. A few notes:

1. The flag in the parent process mDocShellIsActive is initialized to false in ContentParent.cpp, which is wrong. The "real" initial state for a BrowsingContext/DocShell is true

• to be even more correct it should probably just directly read and set the BrowsingContext::{Get,Set}IsActive synced field.

2. The flag is set from the content process by sending a BrowsingContext::SendSetDocShellIsActive message, and then the content process setting the flag on the BC.

• We should be directly setting it on the BC from the parent process, and not exposing the flag on BrowserParent or nsDocShell

3. The actual side effects of setting IsActive are only occurring in the nsDocShell setter method, and aren't happening in a consistent way across all processes. This means that, e.g. after a process switch, the 3 different states (BrowserParent, BrowsingContext, and in-process state) don't necessarily match at all.

This entire setup probably needs to be reworked to instead work with isActive being set on a frame from the parent process, and the state being correctly set and the callbacks being correctly fired in every content process including after process switches.

ni? :emilio, as they've worked on issues like this one before.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Here's a first pass... https://treeherder.mozilla.org/#/jobs?repo=try&revision=93a210f945003165813f6f801ad75b1eed4451d1

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1635914 - Move active flag handling explicitly to BrowsingContext. r=nika

And have it mirror in the parent process more automatically.

The docShellIsActive setter in the browser-custom-element side needs to
be there rather than in the usual DidSet() calls because the
AsyncTabSwitcher code relies on getting an exact amount of notifications
as response to that specific setter. Not pretty, but...

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Sorry I slacked a bit on this, I saw a lot of orange and I tried to fix the tab switcher in more elegant ways, oh well!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ef67464ede7
Move active flag handling explicitly to BrowsingContext. r=nika

Comment 12

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/6ef67464ede7

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1635914 - Remove GeckoView workaround. r=agi,snorp

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/012e6d8a9513
Remove GeckoView workaround. r=agi

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/012e6d8a9513