1186892 - Strikethrough of "http" appears broken when protection is disabled on pages with insecure content

Status: VERIFIED FIXED

(Firefox :: Security, defect, P4)

Description

[Catalin Varga [QA][:VarCat]]

Attachment: visual_bug.jpg

Evironment:

Win 7 x64
FF 42
Build Id: 20150722030205

STR:

1. Go to https://www.bennish.net/mixed-content.html
2. Disable protection for insecure content.

Issue:
The strikethrough over http is interrupted. For a better explanation of the bug please check the attached screenshot.

Comment 1

[u279076]

Moving this to Firefox :: Security to match the parent bug.

Comment 2

[Tanvi Vyas[:tanvi]]

Perhaps this is addressed by https://bugzilla.mozilla.org/show_bug.cgi?id=1277937 ?

Comment 3

[Paul Silaghi, QA [:pauly]]

This bug is still reproducible on 50.0a1 (2016-06-09) Win 7

Comment 4

[Johann Hofmann [:johannh]]

Xidorn, do you know if this could be related to what you fixed in Bug 1277937?

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Not sure what's wrong here. Is the issue about that the strikethrough is a dashed line rather than a solid gray line?

That is an issue unrelated to bug 1277937. And it seems the code for this doesn't really consider the use of selection decoration here. Could anyone tell me what is the expected effect? Then I can probably fix this.

Comment 6

[Paul Silaghi, QA [:pauly]]

(In reply to Xidorn Quan [:xidorn] (UTC+1) from comment #5)
> Not sure what's wrong here. Is the issue about that the strikethrough is a
> dashed line rather than a solid gray line?
Yes

Comment 7

[Tanvi Vyas[:tanvi]]

The https should have a full line strikethrough instead of the dashes.  I'm not sure what caused this regression, but it seems like it has been around for a while and we need to fix it.

Comment 8

[Paul Silaghi, QA [:pauly]]

Note that it only reproduces on Windows.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

It seems to me the whole selection decoration mechanism in nsTextFrame never considers the URLStrikeout selection type. I guess it was initially only used for IME. And consequently you can see lots of warnings printed to output about invalid underline style when the strike line there is displayed.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

The selection decoration code looks pretty broken with the additional URL strikeout... But given that is only used internally, and in only one place, I guess it isn't necessary to fix everything now... I think it does need some cleanup eventually, though...

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1186892 - Handle url strikeout separately from IME selection types.

Review commit: https://reviewboard.mozilla.org/r/59578/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59578/

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking to make sure this lands. We could also consider uplifting the fix.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 8763415 [details]
Bug 1186892 - Handle url strikeout separately from IME selection types.

https://reviewboard.mozilla.org/r/59578/#review56704

Yes, this makes sense AFAICS.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Tracking to make sure this lands. We could also consider uplifting the fix.

I would rather not uplift this fix. This bug is a long time and minor issue, not a recent regression. The change here isn't that trivial, especially given that the related code seems broken to me, and I don't believe it has enough coverage from automatic tests, so I suppose there exists certain risk.

Comment 15

[Johann Hofmann [:johannh]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> > Tracking to make sure this lands. We could also consider uplifting the fix.
> 
> I would rather not uplift this fix. This bug is a long time and minor issue,
> not a recent regression. The change here isn't that trivial, especially
> given that the related code seems broken to me, and I don't believe it has
> enough coverage from automatic tests, so I suppose there exists certain risk.

Yeah, I'm pretty sure not uplifting is fine with fxprivacy

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96070b513deb
Handle url strikeout separately from IME selection types. r=jfkthame

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96070b513deb

Comment 18

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 50.0a1 (2016-06-21) Win 7

Comment 19

[Gerry Chang [:gchang]]

Hi Xidorn
Since this patch also affects 49, are you also considering to uplift this patch to 49?

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Given comment 14 (and comment 15), I'm not going to uplift this patch.

Comment 21

[Gerry Chang [:gchang]]

Per comment #14 & #15, the bug has been there for a while. Mark 49 as won't fix.