Closed Bug 1624550 Opened 1 year ago Closed 10 months ago

Redirect-to-download links no longer marked as visited

Categories

(Core :: Networking, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla78
Tracking Status
firefox-esr68 --- unaffected
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- verified

People

(Reporter: emk, Assigned: u480271)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [necko-triaged])

Attachments

(5 files)

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

Flags: needinfo?(matt.woodrow)
Priority: -- → P1
Whiteboard: [necko-triaged]

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.

Flags: needinfo?(matt.woodrow)
Assignee: nobody → dglastonbury

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

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.

For use by both nsDocShell and DocumentLoadListener.

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

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.

Blocks: 1425410
Pushed by dglastonbury@mozilla.com:
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

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.

Flags: needinfo?(dglastonbury)
Flags: needinfo?(dglastonbury)
Flags: qe-verify+

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.