1682285 - Loadstate's external flag goes missing when load goes from parent to child and back to parent

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[:Gijs (he/him)]

I've been trying to fix bug 1678255 by using the load info's loadTriggeredFromExternal property.

Unfortunately this appears not to work, because despite the tab addition from browser.js passing the correct value to addTab, which passes it into the loadState which passes it to webnavigation, which passes it to the browsingcontext.

That passes it down to the child, which ends up in nsDocShell::LoadURI, where it calls CalculateLoadURIFlags which seems to wipe the "from external" information, which then doesn't end up in the load info or loadstate that gets sent back to the parent via document channel construction, which means that by the time we get to the external helper app service in the parent, we no longer have this information and can't use it to make decisions.

I'm not sure how this is supposed to work, but it clearly doesn't right now... Nika or Matt, can you help clarify what is supposed to happen and how best to address this? Happy to write a patch but I'm a bit lost at the moment - is the roundtrip to the child even expected? Should we add an INTERNAL_ docshell flag for the LOAD_FLAGS_FROM_EXTERNAL flag, or is it meant to be preserved some other way (how?).

In terms of STR:

1. ./mach run --remote
2. once the browser finishes starting up, set a breakpoint in DocumentChannelParent::Init
3. ./mach run --remote --url 'mailto:[e-mail]'
4. inspect the loadstate / loadinfo that are passed as arguments.

Comment 1

[Matt Woodrow (:mattwoodrow)]

I think we probably do want an internal version of FROM_EXTERNAL to make this work.

CalculateLoadURIFlags is pretty confusing to me, and I think arose from a refactoring of the old docshell code. We may want to look at rewriting this into something easier to follow at some point (ideally not using the same variable for two different sets of flags).

Comment 2

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

I think :mattwoodrow's analysis here is probably right, the easiest fix here would be to also add an internal version of LOAD_FLAGS_FROM_EXTERNAL.

(In reply to Matt Woodrow (:mattwoodrow) from comment #1)

I think we probably do want an internal version of FROM_EXTERNAL to make this work.

Yeah, I think this is probably the easiest solution, though long term it would be better to avoid most of the weirdness with internal & external load flags if we can get away with it.

CalculateLoadURIFlags is pretty confusing to me, and I think arose from a refactoring of the old docshell code. We may want to look at rewriting this into something easier to follow at some point (ideally not using the same variable for two different sets of flags).

It did, in fact, arise from :qdot's refactoring of the old docshell code. It was confusing then, and it continues to be confusing now :-)

Agreed. I've put together a small sketch locally of keeping two separate variables for the two sets of flags, but haven't gotten far enough along to post it. When I find the time I'll try to get that up.

Comment 3

[Chris Peterson [:cpeterson]]

Priority P2 because this bug blocks P2 bug 1678255.

needinfo'ing Nika

Comment 4

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

Attachment: Bug 1682285 - Part 1: Split internal and external load flags,

Comment 5

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

Attachment: Bug 1682285 - Part 2: Track LOAD_FLAGS_FROM_EXTERNAL separately from LOAD_TYPE,

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1ec0093e6f1
Part 1: Split internal and external load flags, r=kmag,necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/540aebbdf335
Part 2: Track LOAD_FLAGS_FROM_EXTERNAL separately from LOAD_TYPE, r=kmag

Comment 7

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/e1ec0093e6f1
https://hg.mozilla.org/mozilla-central/rev/540aebbdf335

Comment 8

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

The patch landed in nightly and beta is affected.
:nika, 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 9

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

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #8)

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

This bug doesn't seem important enough to uplift to beta - marking as wontfix.