1611469 - backgroundPageThumbsContent.js mixes nsIRequest and nsIWebNavigation load flags

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

See https://searchfox.org/mozilla-central/rev/d48b18fa27315206bf68d4dac82bc4e27a3f138e/toolkit/components/thumbnails/content/backgroundPageThumbsContent.js#48-52.

This bitfield is being assigned to the docShell's defaultLoadFlags, which should be comprised of nsIRequest load flag bits. The nsIWebNavigation bit that is OR'd in has a value of 0x0040 which happens to be an undefined bit in nsIRequest, so for now I guess it's benign, but this is not enforced anywhere as far as I can tell.

I didn't actually check whether this bit is actually respected as intended by this script by consumers of defaultLoadFlags, but it seems like a fair assumption.

Mark and Drew, hg blame suggests that you might understand the intention of this code? Can you help here? :)

Comment 1

[Drew Willcoxon :adw]

iirc the intention was to bypass history when loading pages in a hidden browser for thumbnails. We don't want to add those pages to history or refer to history when loading them (link coloring, etc.). Like you say, I assume it actually works OK since we have tests for this (iirc) and Mark tested it manually when he wrote the patch. I don't recall any discussion about the mismatch, and the bug doesn't seem to record any. Maybe we should add a proper nsIRequest load flag that has the same value as the nsIWebNavigation one?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

In addition to what Drew said, we also didn't want to reflect a "logged in" state - ie, the page should look as though it was being loaded via a new "private browsing" session.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1611469 - backgroundPageThumbsContent.js mixes nsIRequest and nsIWebNavigation load flags r=markh

The patch ensures we don't pass a nsIWebNavigation load flag to
nsIDocShell.defaultLoadFlags which is supposed to get nsLoadFlags (nsIRequest).
The nsIWebNavigation load flag can be passed to loadURI

Comment 4

[Valentin Gosu [:valentin] (he/him)]

(In reply to Drew Willcoxon :adw from comment #1)

iirc the intention was to bypass history when loading pages in a hidden browser for thumbnails. We don't want to add those pages to history or refer to history when loading them (link coloring, etc.). Like you say, I assume it actually works OK since we have tests for this (iirc) and Mark tested it manually when he wrote the patch. I don't recall any discussion about the mismatch, and the bug doesn't seem to record any. Maybe we should add a proper nsIRequest load flag that has the same value as the nsIWebNavigation one?

Some of the thumbnail unit tests fail when I try to pass the flag to loadURI
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=286734057&revision=a3603abd7a0d4797833a9b1bc6a3e681946a0a1f

TEST-UNEXPECTED-FAIL | toolkit/components/thumbnails/test/browser_thumbnails_bg_basic.js | Uncaught exception - at chrome://mochitests/content/browser/toolkit/components/thumbnails/test/head.js:243 - Error: page-thumbnail:error
TEST-UNEXPECTED-FAIL | toolkit/components/thumbnails/test/browser_thumbnails_bg_captureIfMissing.js | got notification of item being created. - Got 0, expected 1
TEST-UNEXPECTED-FAIL | toolkit/components/thumbnails/test/browser_thumbnails_bg_captureIfMissing.js | Thumbnail should be cached after capture -
TEST-UNEXPECTED-FAIL | toolkit/components/thumbnails/test/browser_thumbnails_bg_captureIfMissing.js | Uncaught exception - [Exception... "Component returned failure code: 0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) [nsIFile.lastModifiedTime]" nsresult: "0x80520006 (NS_ERROR_FILE_TARGET_DOES_NOT_EXIST)" location: "JS frame :: chrome://mochitests/content/browser/toolkit/components/thumbnails/test/browser_thumbnails_bg_captureIfMissing.js :
TEST-UNEXPECTED-FAIL | toolkit/components/thumbnails/test/browser_thumbnails_bg_captureIfMissing.js | A promise chain failed to handle a rejection: page-thumbnail:error - stack: observe@resource://gre/modules/BackgroundPageThumbs.jsm:158:22

I'm inclined to land it without this (so we just don't pass the flag into the wrong place)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7ce86d93798c
backgroundPageThumbsContent.js mixes nsIRequest and nsIWebNavigation load flags r=markh

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/7ce86d93798c