Closed Bug 1663757 Opened 1 year ago Closed 9 months ago

Expose nsDocShell::GetCurrentURI's current value on CanonicalBrowsingContext

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
88 Branch
Fission Milestone M7
Tracking Status
firefox88 --- fixed

People

(Reporter: whimboo, Assigned: nika)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

Right now the WindowGlobalParent only supports the documentURI but not anything around the visible URI. It would be great to also have the latter available, so eg the about:(.*)error prefix is not part of the URI.

As discussed on IRC @kmag suggested visibleURI.

Severity: -- → N/A
Priority: -- → P3
Fission Milestone: --- → M6c
Fission Milestone: M6c → M7

Nika, how complicated would it be to get this implemented? We see a good amount of intermittent failures for Marionette tests because of this missing API. If no-one could work on it maybe you could explain what needs to be done? Thanks.

Flags: needinfo?(nika)

The current code here is doing document.defaultView.location.href, which in turn is getting the current URI for the page as returned from nsDocShell::GetCurrentURI. The code which maintains this state is kinda weird and sometimes hard to follow, so it might be a bit of work to come up with a clean solution to reflect it into the parent process.

The simplest solution here would be to reflect it into the parent process by sending a message from nsDocShell::SetCurrentURI, but we may want to somehow fuse it with the dispatching of the OnLocationChange web progress listener event in cases where we can do so, so that the current URI according to OnLocationChange callbacks, and the one recorded on the BrowsingContext don't get out-of-sync.

It might be worth doing some reading into the behaviour of this value to see if we can derive it in the parent process in a cleaner way than we do currently, as it generally should line up with the document URI with a few exceptions.

Tom, is this something you can work on?

Flags: needinfo?(nika) → needinfo?(ttung)

(In reply to Nika Layzell [:nika] (ni? for response) from comment #2)

The simplest solution here would be to reflect it into the parent process by sending a message from nsDocShell::SetCurrentURI, but we may want to somehow fuse it with the dispatching of the OnLocationChange web progress listener event in cases where we can do so, so that the current URI according to OnLocationChange callbacks, and the one recorded on the BrowsingContext don't get out-of-sync.

Nika, could you elaborate more on "the one recorded on the BrowsingContext"? I am not sure if I understand that correctly. Thanks in advance!

Skimmed through some pieces of code, I found that we have BrowserChild::OnLocationChange and BrowserParent::RecvOnLocationChange (which calls GetBrowsingContext()->Top()->GetWebProgress()->OnLocationChange(). They are used to sync OnLocationChange event through the content and the parent processes.

I think I need to check if nsDocLoader::FireOnLocationChange which is called by nsDocShell::SetCurrentURI actually calls BrowserChild::OnLocationChange. If that is true, we can probably just set WindowGlobalParent::mVisableURI in BrowsingContextWebProgress::OnLocationChange on the parent process and thus the value won't be out-of-sync.

Flags: needinfo?(ttung) → needinfo?(nika)
Assignee: nobody → ttung

I think I need to check if nsDocLoader::FireOnLocationChange which is called by nsDocShell::SetCurrentURI actually calls BrowserChild::OnLocationChange. If that is true, we can probably just set WindowGlobalParent::mVisableURI in BrowsingContextWebProgress::OnLocationChange on the parent process and thus the value won't be out-of-sync.

BrowserChild adds itself into docshell's web progress listener so that it's guaranteed to be notified when nsDocShell::SetCurrentURI is called.

Taking this bug as I mention on the review as fixing it will require tweaks to our remote web progress infrastructure.

Assignee: ttung → nika
Flags: needinfo?(nika)
Attachment #9200869 - Attachment is obsolete: true
Depends on: 1691410
Summary: Add support for visibleURI to WindowGlobalParent to sync the visible location of the child → Expose nsDocShell::GetCurrentURI's current value on CanonicalBrowsingContext

This change allows associating individual web progress events with which frame
they originate from, rather than only recording the isToplevel information as
we were before, which is necessary in order to use the OnLocationChange events
from content to track the current URI on CanonicalBrowsingContext.

Due to events in ContentChild being filtered through nsBrowserStatusFilter, some
of the callbacks will never be passed nsIRequest or nsIWebProgress pointers, and
this patch also simplifies them by removing information which is not necessary
from the IPC message.

Finally, this patch adds a number of checks to the relevant Recv callbacks in
the parent process which help ensure that it does not accept web progress events
from a content process which is no longer hosting the target BrowsingContext.
This may allow for us to stop manually suspending web progress events on process
switches, as these checks will handle this automatically based on the existing
BrowsingContext and WindowContext objects.

Previously, we would need to suspend progress events from the previous
BrowserParent, as otherwise we would receive STATE_STOP progress notifications
from the previous browser when it is destroyed, which would throw off frontend
code. With the new checks added by part 1, we will now catch these cases by
detecting that the current window global has changed, and we can get rid of this
explicit override.

Previously, we would only send web progress events from the toplevel
BrowserParent, as other frames would never have the browser-child.js framescript
loaded in them, and so would never start sending events. This change moves the
decision to begin sending events into BrowserChild itself around the same time
as it would've happened previously with the framescript.

This new callsite should still avoid sending events for the creation of the
initial about:blank document in the BrowserChild, while not skipping any other
events, as before.

This URI is intended to reflect the currentURI field on the content nsIDocShell,
and is the value used for getters like window.location.href. For most
documents, this generally matches the Document URI on WindowGlobalParent, it
does not match in specific cases such as error pages or when performing a
session restore.

The field is kept up-to-date by listening to OnLocationChange notifications
from the content process, and is ignored when the BrowsingContext is loaded in
the parent process.

This specifically tests the interesting case of loading an error page.

Duplicate of this bug: 1678430
Status: NEW → ASSIGNED
Pushed by nlayzell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10a646d0c74a
Part 1: Include BrowsingContext info in WebProgressData, r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/1f0e13aecced
Part 2: Don't suspend webProgress on process switch, r=annyG,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/ce268e67dbcd
Part 3: Start sending web progress events in oop subframes, r=annyG
https://hg.mozilla.org/integration/autoland/rev/7560f4f2293f
Part 4: Track current remote URI on CanonicalBrowsingContext, r=annyG,farre
https://hg.mozilla.org/integration/autoland/rev/8657e72a09f7
Part 5: Add a test for CanonicalBrowsingContext.currentURI, r=annyG
Regressions: 1699467
No longer regressions: 1699467
Regressions: 1709346
You need to log in before you can comment on or make changes to this bug.