1624550 - Redirect-to-download links no longer marked as visited

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:

1. Navigate to https://emk.name/test/302.cgi.
2. Click the first link to download a file.

Actual result:
The link color is not changed.

Expected result:
The link color should be changed.

Regression range: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=90cf0ce6c916dc56b5b5c0577f468fc5c4e89745&tochange=d9958362b9bd30f00a64627cb6041df808068cb9

Comment 1

[Matt Woodrow (:mattwoodrow)]

This happens because the redirect is handled by DocumentLoadListener, and then we handle the final URI entirely in the parent using ParentProcessDocumentOpenInfo.

We notify the originating docshell that the load has completed via PDocumentChannel::DisconnectChildListeners, which reports the status code, but the redirect isn't exposed. The docshell thinks that the initial load failed, so records no history entries. The download handler adds the entry for the final downloaded URI.

It looks like IHistory::VisitURI just sends the values it gets back to the parent, and doesn't do any handling in the content process.

Given that, I think we should make DocumentLoadListener responsible for calling VisitURI (on redirects, and when it decides to deliver to the docshell), stop sending the redirect list to the content process, and disabling the VisitURI calls in docshell when we're using DocumentChannel.

Comment 2

[u480271]

I'm working on updating history in the parent process from DocumentLoadListener.

Comment 3

[u480271]

Attachment: Bug 1624550 - P5: Fix racy check for SessionHistory update.

This code was relying on SessionHistory update having completed when
GlobalHistory is update. Moving GlobalHistory update to the parent process with
DocumentChannel means that it's possible for the GlobalHistory event to fire
before the SessionHistory is updated in nsDocShell.

Switch to using tab loading complete to signal when it's OK to check
SessionHistory.

Comment 4

[u480271]

Attachment: Bug 1624550 - P1: Move nsDocShell::mUseGlobalHistory to BrowsingContext.

Comment 5

[u480271]

Attachment: Bug 1624550 - P2: Extract AddURIVisit internals into a helper.

For use by both nsDocShell and DocumentLoadListener.

Comment 6

[u480271]

Attachment: Bug 1624550 - P3: Update Global History from DocumentLoadListener in Parent process.

Avoid round-trip Parent->Content->Parent to add visited URI to browsing history.

Comment 7

[u480271]

Attachment: Bug 1624550 - P4: Cleanup APIs for setting BrowsingContext::UseGlobalHistory.

This value is determined in Parent process and passed down to nsDocShell. Delete
the messages to pass the setting down and set it on the BrowsingContext in the
Parent process.

Refactor the code that determines to opt-out of using global history. Code
inspection determines that windowless browsing contexts want to opt-out as well
as any frame with disableglobalhistory attribute set on it.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0724e10f1ec3
P1: Move nsDocShell::mUseGlobalHistory to BrowsingContext. r=farre
https://hg.mozilla.org/integration/autoland/rev/de8271c53335
P2: Extract AddURIVisit internals into a helper. r=mak,farre
https://hg.mozilla.org/integration/autoland/rev/1fdbaad17565
P3: Update Global History from DocumentLoadListener in Parent process. r=mak,farre,mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/80dce45689ec
P4: Cleanup APIs for setting BrowsingContext::UseGlobalHistory. r=farre
https://hg.mozilla.org/integration/autoland/rev/dd1197d4200e
P5: Fix racy check for SessionHistory update. r=kmag

Comment 9

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/0724e10f1ec3
https://hg.mozilla.org/mozilla-central/rev/de8271c53335
https://hg.mozilla.org/mozilla-central/rev/1fdbaad17565
https://hg.mozilla.org/mozilla-central/rev/80dce45689ec
https://hg.mozilla.org/mozilla-central/rev/dd1197d4200e

Comment 10

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

The patch landed in nightly and beta is affected.
:djg, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 11

[Alexandru Trif, Desktop QA [:atrif]]

Reproduced the issue using Firefox 76.0a1 (20200324214641) on Windows 10x64. The issue is verified fixed with Firefox 78.0b7 on Windows 10x64, macOS 10.12 and Ubuntu 18.04.