1587396 - Thunderbird-only read of AppType from root docshell in CanLoadImage uses nsIDocShellTreeItem

Status: NEW

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

Description

[Kannan Vijayan [:djvj]]

https://searchfox.org/mozilla-central/rev/7cc0f0e89cb40e43bf5c96906f13d44705401042/dom/base/nsContentUtils.cpp#3152

Retrieves AppType of root docshell. This state can be synchronized, probably to TabContext.

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Bug 1587396 - part 0: apply format.

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Bug 1587396 - part 1: Move mAppType flag from nsDocShell to BrowsingContext.

Comment 4

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Bug 1587396 - part 2: Fix usage of nsIDocShellTreeItem in nsContentUtils::CanLoadImage().

Comment 5

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

I don't think we need to fix this usage of nsIDocShellTreeItem, as it will always be APP_TYPE_UNKNOWN when running in firefox (https://searchfox.org/mozilla-central/rev/d6f957415cf009995ecb539ef1425316d82164a9/docshell/base/nsIDocShell.idl#270-271), and thunderbird doesn't support e10s or Fission.

I'm sorry for not catching this bug, and moving it out of M6, before you started working on it.

Comment 6

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to :Nika Layzell (ni? for response) from comment #5)

I don't think we need to fix this usage of nsIDocShellTreeItem, as it will always be APP_TYPE_UNKNOWN when running in firefox (https://searchfox.org/mozilla-central/rev/d6f957415cf009995ecb539ef1425316d82164a9/docshell/base/nsIDocShell.idl#270-271), and thunderbird doesn't support e10s or Fission.

I'm sorry for not catching this bug, and moving it out of M6, before you started working on it.

Aha, okay.

By the way, can we postpone other bugs which toucing nsIDocShell::AppType (e.g. bug 1587406, bug 1597454) as the same with this?

Comment 7

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

(In reply to Tetsuharu OHZEKI [:tetsuharu] [UTC+9] from comment #6)

By the way, can we postpone other bugs which toucing nsIDocShell::AppType (e.g. bug 1587406, bug 1597454) as the same with this?

Yes we can. Thanks for catching those other bugs!

Comment 8

[Chris Peterson [:cpeterson]]

Tetsuharu, is this AppType bug and your attached patches still relevant? Fission will soon be enabled by default for all Firefox users.

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

As my understanding, I think this bug is not a blocker for fission by the reason which Nika said in in comment #5.

To eliminate nsIDocShellTreeItem , we would requires changes which are did in my attached patches, but I think that such effort is not a blocker for fission as tracked in bug 1607591. https://searchfox.org/mozilla-central/rev/997b16c37aa3471386098a5ab78e0db486df8973/dom/base/nsContentUtils.cpp#3455

Comment 10

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I unassigned me once.

Comment 11

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

The following patches are waiting for review from an inactive reviewer:

ID	Title	Author	Reviewer Status
D67173	Bug 1587396 - part 0: apply format.	tetsuharu	nika: Resigned from review
D67174	Bug 1587396 - part 1: Move mAppType flag from nsDocShell to BrowsingContext.	tetsuharu	nika: Resigned from review
D67175	Bug 1587396 - part 2: Fix usage of nsIDocShellTreeItem in nsContentUtils::CanLoadImage().	tetsuharu	nika: Resigned from review

:tetsuharu, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.