Closed Bug 1703289 Opened 4 years ago Closed 4 years ago

Fix browser/base/content/test/siteIdentity/browser_bug902156.js for Fission+BFCache

Categories

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

defect

Tracking

()

RESOLVED FIXED
89 Branch
Fission Milestone M7a
Tracking Status
firefox89 --- fixed

People

(Reporter: neha, Assigned: emilio)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This fails in UI but probably happening because platform isn't setting the right flags for UI's consumption, maybe in CanonicalBrowsingContext::UpdateSecurityState() at https://searchfox.org/mozilla-central/source/docshell/base/CanonicalBrowsingContext.cpp#231

Failure log: https://treeherder.mozilla.org/logviewer?job_id=332689903&repo=try&lineNumber=5483

Prefs to set: fission.autostart and fission.bfcacheInParent

Severity: -- → S3
Fission Milestone: --- → M7a
Flags: needinfo?(emilio)
Priority: -- → P2

Or are we missing a call to UpdateSecurityState() somewhere?

So this is a bit more tricky than that. Afaict, the issue is that we don't persist the "allow mixed content" bit on the window. That comes from this code, which looks at whether the docshell has a mixed content channel.

With bfcacheInParent, the docshell for the new window is different, so there's no mixed content channel in the new docshell. I guess this can be an issue for regular fission as well. If the expectation is that the "allow mixed content" bit is preserved across all navigations in the same tab, including cross-origin, then that state needs to move to the browser element... or the browsing context + preserved on switch I guess.

So, right now the mixed-content channel is stored on the docshell, and does a same-origin check, so that state is persisted only across same-origin navigations.

That is certainly not impossible to implement with bfcache-in-parent, but it is a bit annoying because the docshell can change even for same-origin navigations.

Things we could do here:

Disable bfcache when the user allows mixed content

Easiest, but depends on not many loads doing this. Tanvi, do you know if we have telemetry on this sort of stuff?

Bite the bullet and implement it...

Even if it's some pretty weird state preserved for navigation. Probably doable, but not super-duper-amazing.

Turn the "allow mixed content" bit into a "usual" per-site permission?

That seems to be what Chromium does (Safari doesn't seem to allow loading mixed content at all, I think).

Tanvi / Paul / Tim, do you know if this has been discussed before? It might be more reasonable behavior than the current one, specially now that the mixed-content-enabling ui is hidden at first-sight (we show the regular lockpad).

Happy to go either way, or if I may have missed some other option, please let me know :)

Flags: needinfo?(tihuang)
Flags: needinfo?(tanvi)
Flags: needinfo?(pbz)
Flags: needinfo?(emilio)

I talked a bit with Christoph about this, and the telemetry for this on release is non-zero. But he agreed with me that the current behavior of only persisting it for one set of same-origin navigations (not even per-session) is pretty nasty, so converting it to use the permission manager like we do for https-only mode etc seems like it would be a good improvement long-term, even if it's a bit more work. I'll give something like that a shot.

Flags: needinfo?(tihuang)
Flags: needinfo?(tanvi)
Flags: needinfo?(pbz)
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)

This is more fission-compatible, and a lot simpler.

Assignee: nobody → emilio
Status: NEW → ASSIGNED
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d54f14759b86 Make mixed content blocker use a session-persistent permission rather than ad-hoc code. r=ckerschb,smaug
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: