1586830 - Set WindowGlobalParent::IsInitialDocument correctly

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P3)

Description

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

I was looking at WindowGlobalParent, and came across the mIsInitialDocument field which was apparently added in bug 1543251. This appears to currently be set incorrectly. In order to fix this, I think we need to change a few things:

1. This bit should be set on creation time for documents created with WindowGlobalActor::AboutBlankInitializer, as these windows are always initially for initial documents.
2. This bit must be cleared as soon as the initial about:blank document is replaced in the content process. Currently once the bit is set, it is never unset, even once another non-initial document has been loaded. (with the one exception being a call to document.open()). This should probably be done in nsGlobalWindowInner::InnerSetNewDocument. (In general we probably need a SetNewDocument callback on WindowGlobalChild to update various state. I'm a touch worried that the DocumentURI isn't being updated correctly in this case either, now that I'm seeing the code again).

Comment 1

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

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

For more information, please visit auto_nag documentation.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

I can work on that, but what does "initial document" mean exactly?

Comment 3

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

When an <iframe> is created, it is possible to immediately access .contentDocument, in which case you will see an empty about:blank document. This is an initial about:blank document, which is replaced by the real document after it is loaded. We also create these when creating other toplevel browsing contexts, such as for window.open().

The interesting property of this type of document is that, when it is navigated to another same-origin document, that document shares the same Window object as this about:blank document (for historical reasons).

Comment 4

[David Teller [:Yoric] - still alive but not very active]

So, it should be true initially, then false once we start loading a new document?

Comment 5

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

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #4)

So, it should be true initially, then false once we start loading a new document?

Yes, but only for initial about:blank documents, such as those created with WindowGlobalActor::AboutBlankInitializer.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attaching a prototype. Is this what you had in mind?

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1586830 - Fix IsInitialDocument;r?nika

Comment 8

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

replied on the patch

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1586830 - Make sure that IsInitialDocument becomes false once it's not the initial document anymore;r?nika

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Do we have any way to test this property?

Comment 11

[Chris Peterson [:cpeterson]]

Deferring to Fission Nightly (M6)

Nika provided review comments. Yoric is waiting for Nika to answer some follow-up questions.

Comment 12

[Chris Peterson [:cpeterson]]

Tracking for Fission M6b

Comment 13

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

The main remaining issue here is just that we don't initialize this value to true when creating the temporary dummy initial window actors which are created before the content process is initialized and we don't set the flag quite as early as it might be nice to. That being said, this field isn't used in very many places right now, and I think it's highly unlikely that we're going to run into any issues in the current use places, so I'm going to deprioritize this for now.

Putting in MVP so we can re-assess where it's used and how important the work is before we ship.

Comment 14

[Chris Peterson [:cpeterson]]

Putting in MVP so we can re-assess where it's used and how important the work is before we ship.

kmag says he found a few cases that are broken and should be fixed before Fission MVP.

Some cases:
DocumentLoadListener
Service Worker code

Comment 15

[Chris Peterson [:cpeterson]]

Nika says the fix will be tricky and is not a super-high priority unless it’s helpful for some other reason. DevTools wants something like this fix.

Comment 16

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

Attachment: Bug 1586830 - Part 1: Ensure IsInitialDocument is set earlier and consistently on WindowGlobalParent, r=smaug

This makes sure to clear and set the value more consistently when replacing
documents within a WindowGlobal, and makes sure to include the relevant flag in
the initializer.

In addition, the place where the flag is set is moved ahead to happen before
the call to Embed so that the information is ready before the window is
created.

Comment 17

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

Attachment: Bug 1586830 - Part 2: Expose Document.isInitialDocument to chrome JS, r=smaug

Previously only the value in the parent process,
windowGlobalParent.isInitialDocument, was exposed to chrome JS. for testing
and other purposes, it is also useful to be able to read this value from JS in
a child process.

Depends on D119815

Comment 18

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

Attachment: Bug 1586830 - Part 3: Add a test for isInitialDocument, r=smaug

This test is based on the test written by ochameau for
https://phabricator.services.mozilla.com/D118740, but with modifications
to specifically test isInitialDocument.

Depends on D119816

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/615b15674468
Part 1: Ensure IsInitialDocument is set earlier and consistently on WindowGlobalParent, r=smaug
https://hg.mozilla.org/integration/autoland/rev/38b111a76a73
Part 2: Expose Document.isInitialDocument to chrome JS, r=smaug
https://hg.mozilla.org/integration/autoland/rev/3e6182be78c7
Part 3: Add a test for isInitialDocument, r=smaug

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/615b15674468
https://hg.mozilla.org/mozilla-central/rev/38b111a76a73
https://hg.mozilla.org/mozilla-central/rev/3e6182be78c7