1675916 - Get rid of ClientSource::MaybeCreateInitialDocument

Status: RESOLVED INVALID

(Core :: DOM: Service Workers, task, P3)

Description

[Jens Stutte [:jstutte]]

This function has no real context in its class that justifies to be here.

Comment 1

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

The component has been changed since the backlog priority was decided, so we're resetting it.
For more information, please visit auto_nag documentation.

Comment 2

[Jens Stutte [:jstutte]]

Attachment: Bug 1675916 - Get rid of ClientSource::MaybeCreateInitialDocument r=#dom-workers-and-storage

Comment 3

[Jens Stutte [:jstutte]]

(waiting for try push)

Comment 4

[Jens Stutte [:jstutte]]

Collecting some try pushes...

1. Just deleting MaybeCreateInitialDocument try result

2. Adding a docshell->GetDocument() to BrowsingContext::SetDocShell (in case of content only) try result

3. Same as 2) but only for XRE_IsContentProcess() try result

Comment 5

[Jens Stutte [:jstutte]]

1. did not execute WPT tests, it seems, here a push with only WPT.

It is clear, that 2) and 3) are causing far too much collateral damage to be further investigated.

So we should concentrate on the test failures caused by 1)

service-workers/service-worker/about-blank-replacement.https.html
There is an intermittent bug 1443414 for this that disabled a similar check for popup case. Now it is permafailing also here.

service-workers/service-worker/fetch-event-handled.https.html
service-workers/service-worker/fetch-event-worker-timing-frame.tentative.https.html
NOT RUN ?

Comment 6

[Jens Stutte [:jstutte]]

Pernosco session of the about-blank-replacement.https.html failure

Comment 7

[Jens Stutte [:jstutte]]

And a pernosco session of fetch-event-handled.https.html failure.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jens Stutte [:jstutte] from comment #6)

Pernosco session of the about-blank-replacement.https.html failure

Saving my Slack comments from this trace:

• It seems like the about:blank iframe is getting reported, but its storage access is returning false so it's getting filtered out of the matchAll results list in the ServiceWorker's content process. I added :boom::boom::boom: in the trace for that exact point.
• It seems like the comment at https://searchfox.org/mozilla-central/rev/50215d649d4854812837f1343e8f47bd998dacb5/dom/clients/manager/ClientSource.cpp#320-322 explains what's going on here. The DocShell needs to get upgraded to an inner window via WindowExecutionReady. So the questions would be:
  • Is the test bad and the test should actually be helping force that to happen somehow? The spec around how about:blank works I believe has been improved/formalized since when Ben Kelly was originally implementing this, and it's possible the test may be assuming behaviors that aren't actually specified.
  • Do we need to be forcing the upgrade to a window somewhere? Either in general for the about:blank iframe, or when the MatchAll request causes it to want to snapshot itself? (I think this is what the original logic was trying to do before the recent changes?) As previously discussed, it might be the case that there are 2 cases:
    1. When the document is new, it could be appropriate to upgrade / bring the window into existence.
    2. When the document has explicitly been destroyed, as evidenced by the ClientSource entering shutdown, it could be very inappropriate and the original bug to try and bring the window into existence. (And the fix would be to avoid the teardown ping-pong for ClientSource, instead allowing for snapshot state requests to be dropped because the actor is already dead when the message arrives. Or the teardown needs to be more stateful to explicitly ignore snapshot requests, etc.)
  • Do we need to just be lying and reporting the document like it had a window and depend on something later to cause it to become real?

Comment 9

[Jens Stutte [:jstutte]]

It seems that we cannot get rid of this function here without major changes to our logic we do not want to do.