1597483 - Audit nsIDocShellTreeItem usage in ShowStatus in layout/mathml/nsMathMLmactionFrame.cpp

Status: NEW

(Core :: Layout, defect, P2)

Description

[Kannan Vijayan [:djvj]]

In file layout/mathml/nsMathMLmactionFrame.cpp

https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/layout/mathml/nsMathMLmactionFrame.cpp#231

Called in MathML code, not super perf sensitive.

Gets DocShell->TreeOwner->BrowserChrome, and calls SetStatus on it.

Called on MouseOver and MouseOut.

MouseOver and MouesOut implies high-frequency, but MathML implies low-use rates.

Need someone more informed about the subject to make a call about what to do here.

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Chris Peterson [:cpeterson]]

Moving to Layout.

Please 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 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 3

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jwatt, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Jonathan Watt [:jwatt]]

Erik, would you mind taking a look at this? It would be good for you to have some knowledge of place BrowsingContext plays in our code. :)

Comment 5

[Erik Nordin [:nordzilla]]

Sure! I would be happy to look into this.

Comment 6

[Erik Nordin [:nordzilla]]

So I ran

./mach wpt testing/web-platform/tests/mathml
./mach wpt testing/web-platform/tests/mathml --enable-webrender --enable-fission

and got the following results respectively:

Without Fission

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 2164 checks (1 asserts, 1981 subtests, 182 tests)
Expected results: 2160
Unexpected results: 4
  test: 4 (4 fail)

With Fission

web-platform-test
~~~~~~~~~~~~~~~~~
Ran 2162 checks (1 asserts, 1974 subtests, 187 tests)
Expected results: 2162
Unexpected results: 0

I'm assuming that fact that more tests failed without fission is probably just intermittent test failures. I was hoping for a test case that passes without fission and does not pass with fission so that I can start looking into amending this. I didn't find one here, unfortunately.

Does anyone have an idea for a test case that would explicitly trigger something wrong with nsIDocShellTreeItem in this particular file?

Comment 7

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

It looks like this particular user of nsIDocShellTreeItem::GetTreeOwner is actually fine, so moving this to the rmrf-docshell-tree-item metabug.

Sorry about that.