1637487 - Refactor MixedContentBlocker::ShouldLoad to remove duplicated logic

Status: RESOLVED FIXED

(Core :: DOM: Security, task, P3)

Description

[Matt Woodrow (:mattwoodrow)]

The logic in ShouldLoad for dealing with adding/removing flags seems a lot more complex than it needs to be.

I have a set of patches to try factor out the logic and reduce the duplication, which I think will make it easier to reason about for adding fission support.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1637487 - GetState is infallible, so rely on the result. r?ckerschb

We previously were checking the nsresult inconsistently.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1637487 - Refactor calls to OnSecurityChange to all be a single call at the end of ShouldLoad. r?ckerschb

Depends on D75018

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1637487 - Flatten the mixed content event runner into the calling code, so it's no longer needed. r?ckerschb

This looks like it was necessary a long time ago, but now just runs the same calls as the calling code, so unnecessarily splits the logic into two.

Depends on D75019

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1637487 - Fold the branches for each pref together, since they do the same thing. r?ckerschb

Depends on D75020

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1637487 - Factor out setting of the broken security state flags. r?ckerschb

Factor out broken flag clearing

Depends on D75021

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1637487 - Factor out updating root Document security flags. r?ckerschb

Depends on D75022

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1637487 - Factor out calls to log changes to mixed content security state. r?ckerschb

Depends on D75023

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1637487 - Compute broken flag when we need to use it. r?ckerschb

Depends on D75024

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1637487 - Store mixed content security flags as a flags word on Document instead of bools. r?ckerschb

Depends on D75025

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b83f49bc11b0
GetState is infallible, so rely on the result. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/ab9df2f2d11f
Refactor calls to OnSecurityChange to all be a single call at the end of ShouldLoad. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/83362625c297
Flatten the mixed content event runner into the calling code, so it's no longer needed. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/286e5ee6517f
Fold the branches for each pref together, since they do the same thing. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/d1709ebb7f46
Factor out setting of the broken security state flags. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/1a64ef10e773
Factor out updating root Document security flags. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/65bb1114df87
Factor out calls to log changes to mixed content security state. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/c4f0c757cae0
Compute broken flag when we need to use it. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/5633cc2aaa9b
Store mixed content security flags as a flags word on Document instead of bools. r=ckerschb

Comment 11

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on nsMixedContentBlocker.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/efbb5dd6bf7baeaee6a85520791aa051571786cb
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=302176699&repo=autoland&lineNumber=34561

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6da9185be30
GetState is infallible, so rely on the result. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/a070e28e1293
Refactor calls to OnSecurityChange to all be a single call at the end of ShouldLoad. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/54e745466a1e
Flatten the mixed content event runner into the calling code, so it's no longer needed. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/9edfcabb1fb6
Fold the branches for each pref together, since they do the same thing. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/0d2c833ac4d3
Factor out setting of the broken security state flags. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/10b5c92f0200
Factor out updating root Document security flags. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/d615ede24737
Factor out calls to log changes to mixed content security state. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/7b3d6f15776b
Compute broken flag when we need to use it. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/56310eefdcdd
Store mixed content security flags as a flags word on Document instead of bools. r=ckerschb

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/a6da9185be30
https://hg.mozilla.org/mozilla-central/rev/a070e28e1293
https://hg.mozilla.org/mozilla-central/rev/54e745466a1e
https://hg.mozilla.org/mozilla-central/rev/9edfcabb1fb6
https://hg.mozilla.org/mozilla-central/rev/0d2c833ac4d3
https://hg.mozilla.org/mozilla-central/rev/10b5c92f0200
https://hg.mozilla.org/mozilla-central/rev/d615ede24737
https://hg.mozilla.org/mozilla-central/rev/7b3d6f15776b
https://hg.mozilla.org/mozilla-central/rev/56310eefdcdd