1558149 - Use `loadFlags` instead of `flags` for flags eventually passed to `loadURI` or `fixupAndLoadURIString`

Status: REOPENED

(Firefox :: General, task, P5)

Description

[:Gijs (out for now; he/him)]

Per my earlier comment in phabricator for LoadInOtherProcess in browser.js:

So, this is hard to see, but LoadInOtherProcess goes through a gazillion other helpers and ends up in ContentRestore.jsm, which ultimately expects a flags property, not a loadFlags property - of course, it then promptly renames that to be a loadFlags property, see https://searchfox.org/mozilla-central/rev/9eb2f739c165b4e294929f7b99fbb4d90f8a396b/browser/components/sessionstore/ContentRestore.jsm#222-229 .

We should update LoadInOtherProcess and the rest of the chain to use loadFlags throughout, and ensure any other consumers of such methods also get updated.

I can mentor this, but it's probably not "good first bug" material.

Comment 1

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

Bugbug thinks this bug is a task, but please change it back in case of error.

Comment 2

[Favour]

Please boss can i be assigned the bug?
Thank you...

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Favour from comment #2)

Please boss can i be assigned the bug?
Thank you...

We don't normally assign bugs until patches are put up. Feel free to start work on this. However, as I noted in comment 0, this probably isn't your best bet for a first bug.

I also note you've commented on some 22 different bugs by now, but I don't see any patches yet - I'd highly recommend actually starting on one of the issues.

Comment 4

[anayocrescent2]

I'll work on it, then most of the bug. almost all the bug are already taken

Comment 5

[anayocrescent2]

I don't mind digging into the bug

Comment 6

[anayocrescent2]

I just confirmed that i should go on with working on this bug from @Favour

Comment 7

[anayocrescent2]

i would like to know if functions like LoadInOtherProcess and sessionrestore are renamed or refactored since 2019

Reasons:
there is a mention in ESR78 codebase https://searchfox.org/mozilla-esr78/search?q=LoadInOtherProcess&path=&case=false&regexp=false
but
but not from ESR91 codebase https://searchfox.org/mozilla-esr91/search?q=LoadInOtherProcess&path=&case=false&regexp=false
so, some refactoring happened in between them

looks like the code is modified by https://hg.mozilla.org/mozilla-central/rev/b5edfb574654

Comment 8

[:Gijs (out for now; he/him)]

Good sleuthing! Yes, some of this code has been removed. But some of the code coping with the duplicate naming is still there:

https://searchfox.org/mozilla-central/rev/e6a03adbf7930ae0cf131cc3274c80b2aae74e51/toolkit/content/widgets/browser-custom-element.js#821

and there are also quiet bugs in the codebase that are hard to detect because of this renaming - for instance, https://searchfox.org/mozilla-central/rev/e6a03adbf7930ae0cf131cc3274c80b2aae74e51/browser/base/content/browser.js#3314 passes flags as a property instead of loadFlags, which means the flags end up completely ignored. Oops! The functionality (bypassing a malware block page) still works because the code below it adds a permission entry for that domain, but it doesn't operate the way it should, and did, before several refactors.

The change in bug 1519365 was trying to rename all the uses of these load flags (so things that use the LOAD_FLAGS_** constants from https://searchfox.org/mozilla-central/source/docshell/base/nsIWebNavigation.idl#127 and friends) to use loadFlags instead of the more generic flags. Renaming all of them was difficult, so some stuff got missed/ postponed to this bug. It's now been so long that some of it has been removed, but some is still left.

As far as I can tell, from looking at https://searchfox.org/mozilla-central/search?q=flags&path=browser%2Fcomponents%2Fsess&case=false&regexp=false , there are no uses in sessionrestore left that use flags where they should be using loadFlags.

So in terms of what to do for this bug... ideally we should be able to remove the code in browser-custom-element.js that fixes up flags to loadFlags, and to update any consumers still using flags for things that are really supposed to be loadFlags (at least the browser.js case I linked above). So you can start with those two bits of code - to find more instances of this going wrong, I pushed a test patch to try that tries to fail tests if they use flags. Unfortunately it's incomplete, it won't catch cases calling browsingContext.loadURI or browsingContext.fixupAndLoadURIString with flags - but hopefully there are fewer of those anyway as those APIs are newer. Here's my trypush: https://treeherder.mozilla.org/jobs?repo=try&revision=1608f7a531538353e1f15570f8ac589b76b6642d .

Comment 9

[:Gijs (out for now; he/him)]

(In reply to :Gijs (he/him) from comment #8)

Here's my trypush: https://treeherder.mozilla.org/jobs?repo=try&revision=1608f7a531538353e1f15570f8ac589b76b6642d .

This compiled fine locally but hit clang warnings on infra... trying again: https://treeherder.mozilla.org/jobs?repo=try&revision=e296c334cbadac8791a770dbf1a78f265e80cc16

Comment 10

[:Gijs (out for now; he/him)]

Let's update the bug summary to be clearer about what we should try to do here.

The trypush didn't flag up any issues. I think this is because (a) I didn't run any tests on mobile and (b) in the main tabbed browser, the code at https://searchfox.org/mozilla-central/rev/137075514eddc08c68ff652e9899da82e8043574/browser/base/content/tabbrowser.js#7019-7020 was preventing any code from hitting the assertions or exception-throwing code that I added.

When I fixed that no tabs loaded! I wrote up a quick patch that addressed that and a few other cases, and pushed that to try:

https://treeherder.mozilla.org/jobs?repo=try&revision=f8803805ec4d54b90839876e1062c800e5af65d7

Raw patch so you can see the changes: https://hg.mozilla.org/try/rev/9edef5ef9f7c3c1f9f495b936a55efcfff0ae3f4

I'm fairly sure there are a handful of places I didn't catch so hopefully those will show up on try as failing tests.

A patch for this bug would need to include the JS changes in the above patch, plus the browser.js case I mentioned in comment 8, plus any remaining cases identified by the trypush (assuming it finally works and flags up some brokenness).

Comment 11

[anayocrescent2]

Please which should i use in adding your name-:
r=:Gijs! or r=Gijs

Comment 12

[anayocrescent2]

Attachment: Bug 1558149 - Use loadFlags instead of flags for flags eventually passed to loadURI or fixupAndLoadURIString r?Gijs

Comment 13

[:Gijs (out for now; he/him)]

(In reply to anayocrescent2 from comment #11)

Please which should i use in adding your name-:
r=:Gijs! or r=Gijs

r=Gijs should work. I left some comments on the patch already. Thanks!

Comment 14

[anayocrescent2]

I am making the necessary changes Immediately

I got errors earlier because i mistakenly added backticks in the commit message
but all is resolved now, let me do the necessary changes

Comment 16

[:Gijs (out for now; he/him)]

It's been a year so I'm resetting the assignee. :-)

Comment 17

[Isaac Lee]

Attachment: Bug 1558149 - Use 'loadFlags' instead of 'flags' for flags eventually passed to 'loadURI' or 'fixupAndLoadURIString' r?Gijs!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6517c084a8e2
Use 'loadFlags' instead of 'flags' for flags eventually passed to 'loadURI' or 'fixupAndLoadURIString' r=Gijs,geckoview-reviewers,tabbrowser-reviewers,dao,owlish

Comment 19

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6517c084a8e2

Comment 20

[:Gijs (out for now; he/him)]

There's more work to do here, we're tackling it iteratively. :-)