Closed Bug 1520791 Opened 11 months ago Closed 11 months ago

"docShellIsActive" is no longer set to "true" for the initial tab after opening the browser

Categories

(Firefox :: Tabbed Browser, defect, P1)

65 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 66
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- unaffected
firefox65 + fixed
firefox66 + verified

People

(Reporter: whimboo, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

Since the patch on bug 1506608 landed the value of docShellIsActive is no longer true for the initial tab.

Steps:

  1. Start Firefox
  2. Open the Scratch pad
  3. Switch to Browser environment
  4. Run: alert(window.gBrowser.selectedTab.linkedBrowser.docShellIsActive)

Expected result:

The flag should report that the docshell is active

Current result:

The flag tells that the docshell is not active

Once a new tab gets opened the value is correctly initialized.

I don't know which implications that generally has and how important it is to get fixed. As such I will mark it as tracking firefox 65.

Flags: needinfo?(gijskruitbosch+bugs)

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

I don't know which implications that generally has and how important it is to get fixed. As such I will mark it as tracking firefox 65.

Me neither. The content docshell does know that it's active, from the looks of it (when checking with the browser content toolbox . But I expect this isn't good for the tabswitcher code...

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mconley)
Priority: -- → P1

tracking65+, but it would be really good to understand what the implications of this are given that we're building the final beta of the 65 cycle today and the RC next week.

Before bug 1506608 landed, we were flipping the remoteness of the initial tab, which caused us to hit this part of the codebase: (1).

shouldActivateDocShell would return true, and bam, the docShellIsActive property would be set to true.

Because we don't flip remoteness for the initial tab (which is a good thing), we never hit this code path, so the property is never touched.

What's particularly troubling is that the TabParent and the underlying DocShell are out of sync - TabParent's mDocShellIsActive member defaults to false (2), but DocShell's mIsActive member defaults to true (3). That's what Gijs was seeing in comment 1.

Given how close we are to the uplift, here's what I suggest:

  1. We paper over the issue the safe way by setting the docShellIsActive property on the TabParent for the initial tab manually.
  2. File a follow-up bug to reconcile the initial active states between TabParent and nsDocShell.
Flags: needinfo?(mconley)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/de634684229e
ensure initial browser's docshell is marked as active in the parent, r=mconley
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Comment on attachment 9037269 [details]
Bug 1520791 - ensure initial browser's docshell is marked as active in the parent, r?mconley

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1506608

User impact if declined: Unknown. Possibly nothing. This patch is addressing an inconsistency that we discovered where something in the parent process is out of sync with the underlying representation in its content process. This has been on Nightly and in Beta for a while, and we haven't heard about any major problems, but we'd rather be safe than sorry.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Not risky, since we're effectively reverting the logic back to what we did before bug 1506608 landed, where we manually set the docShellIsActive state in the parent on the initial tab to true, to match the underlying DocShell isActive.

String changes made/needed: None.

Attachment #9037269 - Flags: approval-mozilla-beta?

Comment on attachment 9037269 [details]
Bug 1520791 - ensure initial browser's docshell is marked as active in the parent, r?mconley

[Triage Comment]
Gets us back to a known-safer state. Patch is low-risk. Approved for 65.0b12.

Attachment #9037269 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Thanks for the quick fix.

Status: RESOLVED → VERIFIED
Blocks: 1520302
You need to log in before you can comment on or make changes to this bug.