1580281 - Audit usage of nsIDocShellTreeItem in nsDocShell::SetupNewViewer

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Kannan Vijayan [:djvj]]

https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8319

Fixing this seems a tad more involved. This code gets the parent docshell via the DocShellTreeItem interface, and then queries it for its ContentViewer (kept in oldCv). It then queries oldCv for the following parameters: GetForceCharset, GetHintCharset, GetHintCharacterSetSource, GetTextZoom, GetFullZoom GetOverrideDPPX, GetAuthorStyleDisabled.

Either these attributes need to be synced to BrowsingContext, or the set of attributes retrieved via IPC.

If this needs to be made into IPC and async IPC is a requirement, it seems like the main cutpoint is two calls above (via nsDocShell::Embed) to nsDocShell::CreateContentViewer (and CreateAboutBlankContentViewer).

@Nika - ^-- does the above sound reasonable?

I've copy-pasted the relevant bits of code below for easy reference:

   nsCOMPtr<nsIDocShellTreeItem> parentAsItem;
  NS_ENSURE_SUCCESS(GetInProcessSameTypeParent(getter_AddRefs(parentAsItem)),
                    NS_ERROR_FAILURE);
  nsCOMPtr<nsIDocShell> parent(do_QueryInterface(parentAsItem));
  ...
    parent->GetContentViewer(getter_AddRefs(oldCv));
  ...
    if (oldCv) {
      newCv = aNewViewer;
      if (newCv) {
        forceCharset = oldCv->GetForceCharset();
        hintCharset = oldCv->GetHintCharset();
        NS_ENSURE_SUCCESS(oldCv->GetHintCharacterSetSource(&hintCharsetSource),
                          NS_ERROR_FAILURE);
        NS_ENSURE_SUCCESS(oldCv->GetTextZoom(&textZoom), NS_ERROR_FAILURE);
        NS_ENSURE_SUCCESS(oldCv->GetFullZoom(&pageZoom), NS_ERROR_FAILURE);
        NS_ENSURE_SUCCESS(oldCv->GetOverrideDPPX(&overrideDPPX),
                          NS_ERROR_FAILURE);
        NS_ENSURE_SUCCESS(oldCv->GetAuthorStyleDisabled(&styleDisabled),
                          NS_ERROR_FAILURE);
      }
    }

Comment 1

[Kannan Vijayan [:djvj]]

Forgot to CC nika on question in top comment.

Comment 2

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

(In reply to Kannan Vijayan [:djvj] from comment #0)

https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8319

Because this isn't a permalink, it starts pointing to the wrong line of code over time. I think the line you meant to reference is https://searchfox.org/mozilla-central/rev/588814f2edddf0e132d77d326ddae50911e8bad1/docshell/base/nsDocShell.cpp#8341

Fixing this seems a tad more involved. This code gets the parent docshell via the DocShellTreeItem interface, and then queries it for its ContentViewer (kept in oldCv). It then queries oldCv for the following parameters: GetForceCharset, GetHintCharset, GetHintCharacterSetSource, GetTextZoom, GetFullZoom GetOverrideDPPX, GetAuthorStyleDisabled.

Either these attributes need to be synced to BrowsingContext, or the set of attributes retrieved via IPC.

These can probably be sent alongside the other load context properties on the BrowsingContext object. Right now we pass this around with TabContext, but we're considering adding it as a property of the BrowsingContext. The zoom properties probably can to be synced that way. That being said, some properties like inherited charset shouldn't be used when cross-origin, so it's probably OK to not sync them in this case.

Apparently ForceCharset is thunderbird only (according to :smaug), so we can probably ignore that completely.

Comment 3

[Chris Peterson [:cpeterson]]

Kannan says replacing nsIDocShellTreeItem calls should block enabling Fission in Nightly (M6).

Comment 4

[Chris Peterson [:cpeterson]]

We need to audit this use of the nsIDocShellTreeItem interface. With Fission enabled, Documents and nsDocShells for related frames, such as subframes and parent documents, may not be available within the current process and the corresponding nsIDocShellTreeItem methods will return null.

If this code is broken with Fission, fixing it blocks enabling Fission is Nightly.

If this code works as-is with Fission, we don't need to remove this usage of nsIDocShellTreeItem until when we remove nsIDocShellTreeItem entirely (bug 1607591) after we ship Fission MVP.

Fission documentation about replacing nsIDocShellTree Item:
https://wiki.mozilla.org/Project_Fission/DocShell_Tree_Replace

:farre's presentation with examples of replacing nsIDocShellTreeItem with BrowsingContext, WindowContext, SyncedContexts, and BrowsingContextGroup:
https://docs.google.com/presentation/d/1K4j6ngty64TZjJNS5qH-MBoOm3TI2dJedVsbH8jUhKE/edit#slide=id.g6e35225e5d_1_264

Comment 5

[Chris Peterson [:cpeterson]]

Auditing whether this use of nsIDocShellTreeItem breaks when Fission is enabled blocks Fission Nightly.

Comment 6

[Neha Kochar [:neha]]

Nika said this is most probably already done so marking it fixed.