995801 - EV identifier is no longer displayed in Location Bar/Arrow Panel after a restart of Firefox

Status: VERIFIED FIXED

(Core :: DOM: Security, defect)

Description

[Alice0775 White]

Steps To Reproduce:
1. Open Web pages which have EV identifier
   i.e. https://addons.mozilla.org/en-US/firefox/
        https://bugzilla.mozilla.org/
  --- observe, youcan see green EV identifier
2. Exit Browser And Start Firefox again
3. Restore Previous Session

Actual Results:
  EV identifier is no longer displayed after doing "Restore Previous Session"

Comment 1

[Alice0775 White]

Regression window(m-c)
Good:
https://hg.mozilla.org/mozilla-central/rev/fa098f9fe89c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140323171614
Bad:
https://hg.mozilla.org/mozilla-central/rev/e10a3c75e704
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140324071215
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fa098f9fe89c&tochange=e10a3c75e704


Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa098f9fe89c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140323171813
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7a44e5b762f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140323180013
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fa098f9fe89c&tochange=e7a44e5b762f

Regressed by:
e7a44e5b762f	Monica Chew — Bug 985623: Force url classifier clients to specify which tables to lookup, add a pref to skip hash completion checks (r=gcp)

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

As far as I know, EV has nothing to do with safebrowsing lookups. This is very puzzling.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Checking Nightly, I'm only seeing this behavior in the selected tab. The other tabs restore fine. Makes me doubt even more it's safebrowsing.

Comment 4

[Alice0775 White]

Background tab also affected for me.
Anyway, when there is some such kind bug, I cannot believe Firefox with safety anymore.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The interesting fact is that when you switch between different affected versions, it will work again until the next restart.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

I'm seeing this even in fa098f9fe89c so I suspect the bisection was erronous.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Alice, would you be so kind and give us the URLs of the last good and first broken build? It's a bit of a hassle to find those on the FTP server. I would like to try that range on my own. Thanks.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #6)
> I'm seeing this even in fa098f9fe89c so I suspect the bisection was erronous.

Oh, me too. So we need indeed a new regression range here.

Comment 9

[Alice0775 White]

mozilla-central
Good:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1395620174/
Bad:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1395670335/


mozilla-inbound
Good:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1395620293/
Bad:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1395622813/

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

A quick spot-check showed me that this started to fail sometime between 02/15 - 03/01.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

For me this started to fail between:

Good:
BuildID=20140226030202
SourceRepository=https://hg.mozilla.org/mozilla-central
SourceStamp=626d99c084cb

Bad:
BuildID=20140227030203
SourceRepository=https://hg.mozilla.org/mozilla-central
SourceStamp=a98a1d78817f


Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=626d99c084cb&tochange=a98a1d78817f

Can someone prove that please? Would be good to get a pushlog for inbound builds too.

Comment 12

[Alice0775 White]

I have a different range for Back Ground tab

Steps to reproduce:
1. Open about:home in first tab
2. Open https://addons.mozilla.org/en-US/developers/  in 2nd tab
3. Open https://bugzilla.mozilla.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bug_status=RESOLVED&bug_status=VERIFIED&chfield=[Bug%20creation]&chfieldfrom=-96h&chfieldto=Now&product=Add-on%20SDK&product=Core&product=Firefox&product=mozilla.org&product=Mozilla%20Application%20Suite&product=Thunderbird&product=Toolkit&product=Tech%20Evangelism&product=Mozilla%20Localizations&product=MailNews%20Core&query_format=advanced&order=bug_id&limit=0&list_id=9804264 in 3rd tab and selected
4. Exit Firefox
5. Start Firefox and click "Restore Previous Session"
6. Switch to 2nd Tab

Actual Results:
  No EV identifier in 2nd tab

Regression window(m-c) for background tab:
Good:
http://hg.mozilla.org/mozilla-central/rev/b3c08e6fa790
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140118032128
Bad:
http://hg.mozilla.org/mozilla-central/rev/61fd0f987cf2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140118151828
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b3c08e6fa790&tochange=61fd0f987cf2

Regression window(m-i)
Good:
http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1389980997/
http://hg.mozilla.org/integration/mozilla-inbound/rev/679616f29acc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140117094957
Bad:
http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1389985670/
http://hg.mozilla.org/integration/mozilla-inbound/rev/b887641e3983
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140117110750
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=679616f29acc&tochange=b887641e3983


Regressed by:
b887641e3983	David Keeler — bug 950240 - don't do DV fallback for nsIIdentityInfo.isExtendedValidation r=briansmith

Comment 13

[Alice0775 White]

hah, Firefox is completely broken....

Comment 14

[Gian-Carlo Pascutto [:gcp]]

The first bad revision is:
changeset:   170796:9d58d9a2c8b1
user:        Monica Chew <[e-mail]>
date:        Wed Feb 26 10:51:36 2014 -0800
summary:     Bug 974579: Disable goog-white-digest256 for non-windows (r=gcp)

Now I'm really confused!

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Could it be that SafeBrowsing accessing google.com over HTTPS influences the outcome of the steps to repro? The commit in comment 12 seems much more likely than SafeBrowsing.

Comment 16

[[:mmc] Monica Chew (no longer reading bugmail)]

Alice, when you go to about:config in FF 28 which is affected from comment 12.5, what is the value of urlclassifier.download_allow_table? If it is empty, then the patch from comment 14 shouldn't affect any HTTPS lookups at the start of session restore.

Comment 17

[Alice0775 White]

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #16)
> Alice, when you go to about:config in FF 28 which is affected from comment
> 12.5, what is the value of urlclassifier.download_allow_table? If it is
> empty, then the patch from comment 14 shouldn't affect any HTTPS lookups at
> the start of session restore.

about:config on Firefox28.0
urlclassifier.download_allow_table;goog-downloadwhite-digest256

Comment 18

[[:mmc] Monica Chew (no longer reading bugmail)]

(In reply to Alice0775 White from comment #17)
> (In reply to Monica Chew [:mmc] (please use needinfo) from comment #16)
> > Alice, when you go to about:config in FF 28 which is affected from comment
> > 12.5, what is the value of urlclassifier.download_allow_table? If it is
> > empty, then the patch from comment 14 shouldn't affect any HTTPS lookups at
> > the start of session restore.
> 
> about:config on Firefox28.0
> urlclassifier.download_allow_table;goog-downloadwhite-digest256

:( :( That means the hotfix from bug 985627 was not applied. Also I checked on FF 28 with the hotfix applied and I can reproduce, so my previous comment is incorrect.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

The hotfix won't have been applied if Alice was running Firefox != 27, 28 at the time we pushed it.

Comment 20

[Alice0775 White]

(FYI, I usually use Firefox24esr. I use release/beta/Aurora/Nightly for testing purpose (i.e. very short term))

Comment 21

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Showing the EV indicator depends on having revocation information. Normally this information is available since it has been cached from a recent validation of a TLS connection. If it isn't available, we don't want to fetch it, since the calculation of whether or not to display the indicator is done on the main thread, which means network I/O will block it. In particular, revocation information is not persistently cached (yet - we're working on it), so when loading a page from the cache at startup, an EV site may be presented as non-EV until a network request is made to that site.
Things we may do to change this situation:
- cache EV status
- persistently cache revocation information (again, we're working on it - see bug 993038, although there's not much activity there yet)
- re-work nsSecureBrowserUIImpl (see bug 832834) to perform the EV calculation asynchronously

Comment 22

[Alice0775 White]

Reload(F5 and Ctrl+F5) does not fix after step6 of comment#12.

(I can no longer believe that Firefox is safe. Because, Firefox tells me a lie)

Comment 23

[Gian-Carlo Pascutto [:gcp]]

>(I can no longer believe that Firefox is safe. Because, Firefox tells me a lie)

The behavior described in comment 21 is failsafe: Firefox will not claim the site is safe when it isn't, only the reverse.

This sounds like it is effectively WONTFIX until those bugs land. Although keeler's explanation doesn't clarify why the SafeBrowsing patches affect this. So I'm not sure what to do here.

Comment 24

[Alice0775 White]

Sorry. I can not believe.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Beta 9 go to build is tomorrow (at about 13 PDT), what do we do regarding this bug?

Comment 26

[[:mmc] Monica Chew (no longer reading bugmail)]

Hi Sylvestre, based on comment 21 I don't believe there's anything to do here except wait for bug 993038. I also don't believe that this is a regression because it's present in FF 28.

David, gcp, do you disagree?

Comment 27

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I don't disagree. Caching the EV status might be an acceptable band-aid, but it would be of non-negligible risk to uplift. Also, it would require some expertise from someone familiar with the cache.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

OK so it sounds like we will wontfix this for 29 as well given the timeline, based on Monica and David's comments.

Comment 29

[Gian-Carlo Pascutto [:gcp]]

FWIW, I just filed Bug 1000151 that shows that this can happen even in not-session restore scenarios, and is unrecoverable unless you clear the disk cache. Basically, Firefox's EV indicator is unreliable and hence useless.

Comment 31

[Gian-Carlo Pascutto [:gcp]]

As shown in bug 1000151, this happens without session restore as well, so the explanation in comment 21 probably doesn't hold, and this bug is *far* more serious because EV indicators are just broken.

Updated topic to reflect that.

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So bug 100151 was basically the exact same issue as reported here. Session Restore is not involved at all here. So here updated steps:

1. Create a fresh profile and start Firefox
2. Open https://addons.mozilla.org
3. After the page has been loaded quit Firefox
4. Start Firefox
5. Open https://addons.mozilla.org

With step 5 we do no longer show the EV indicator but a gray lock with an unknown owner. This is pretty bad as pointed out, given that users wont no longer trust websites with EV certificates.

Personally I cleared the disk cache via clear recent history but it didn't work, and I still see the gray lock.

Lukas, if we are shipping with this bug, we should definitely relnote that! What do you think?

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I shouldn't have changed the URLs for testing. So as bug 1000151 says op.fi is way better for testing here. For other domains you might have to get those loaded in a background tab, so also see this behavior. That's what Alice pointed out earlier.

1. Create a fresh profile and start Firefox
2. Type 'op.fi' in the location bar and hit enter
3. After the page has been loaded quit Firefox
4. Start Firefox
5. Type 'op.fi' in the location bar and hit enter
 
Interestingly the EV indicator is still shown when you directly load the URL, op.fi is redirecting to: https://www.op.fi/op?id=10000&_nfpb=true. May this be related to redirects? I'm not sure about the correct pattern.

Oh, and today we talked about this bug in the desktop qa meeting. So Matt Wobensmith will be the QA contact person now.

Comment 34

[Matt Wobensmith [:mwobensmith][:matt:]]

Henrik - this bug is 100% reproducible in shipping Fx28, clean profile, using the op.fi domain. That being the case, why would it be more serious in Fx29? As bad as it is, it hasn't generated any customer complaints that I'm aware of. Please correct me if I'm wrong.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+2]]

op.fi seems to be a special example here. That URL we got today from an user. For other EV websites like AMO it's harder to hit. Here the pages would have to be opened in a background tab. All that are information we noticed after our qa desktop meeting today.

Comment 36

[Gian-Carlo Pascutto [:gcp]]

>As bad as it is, it hasn't generated any customer complaints that I'm aware of.

The testcase came from a user, who noted that they had solved the problem by using Chrome.

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

Moving the ni? to Sylvestre - if there's something unique about the issue on this release over FF28 then we can look at putting it the known issues - flagging relnote-firefox ? as well to keep this on our radar.

Comment 38

[Sylvestre Ledru [:Sylvestre]]

Will be part of the Known issues release notes for 29.

Comment 39

[Nils Ohlmeier [:drno]]

So here is a user complain: for every https web page I have visited before I get the grey icon instead of the green now. All my banking web sites, Google, .. (you name it) have the grey icon since I restarted today to upgrade FF30.

This basically renders the EV feature on FF completely useless. And just pointing to a ticket which has not been assigned, is not marked for any future release, basically a ticket which looks completely idle is IMHO unacceptable. There should be at least a plan to get this fixed in FF 30 before we release it.

Comment 40

[Sylvestre Ledru [:Sylvestre]]

David, any chance to see that bug fixed for 30? Thanks

Comment 41

[Gian-Carlo Pascutto [:gcp]]

If this bug cannot be fixed properly, we should back out bug 9504240. The impact of declining that one was listed as:

>User impact if declined: Jank (user interface freezes for up to a few seconds), poor performance.

I don't see how "poor performance" can weigh up to "EV is now useless".

Comment 42

[Marc Schifer [mschifer]]

If we end up doing a 29.0.1 release can we consider this for inclusion?

Comment 43

[Lukas Blakk [:lsblakk] use ?needinfo]

Yes, we will do a backout of bug 9504240 in order to restore this functionality in the non-urgent 29.0.1

Comment 44

[Lukas Blakk [:lsblakk] use ?needinfo]

That should be bug 950240

Comment 45

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch

This caches the EV status of a certificate on disk, like I tried to do before in another bug without much success. It handles upgrading from an old cache that doesn't include the EV status, but going the other way doesn't work so well.

Comment 46

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Lukas Blakk [:lsblakk] from comment #44)
> That should be bug 950240

Just to close the loop, we were not able to do a backout or forward fix here without adding too much risk to 29.0.1, so this is going to have to wait for FF30

Comment 47

[Gian-Carlo Pascutto [:gcp]]

>going the other way doesn't work so well.

Is updating the name of the cache file an option?

Comment 48

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

1. The long-term solution to this problem and related ones is bug 660749. I understand that this is mostly wallpaper over bug 660749. Do you agree? If so, please add some comments to that effect so that we know.

2. TransportSecurityInfo::Write already has a versioning mechanism that we should be able to utilize here instead of inventing a new versoin detection scheme. What do you think?

3. Rather than trying to gracefully cope with missing serialized EV information in cached responses written by old versions, I think it is OK to simply fail to de-serialize them, as long as that causes the cache to reject (and doom) the entry and fall back to doing a network request. That is, I think it would be fine, for HTTPS cache entries only, to bump the version number written by TransportSecurityInfo::Write and then require that version in TransportSecurityInfo::Read. This would have the effect of invalidating all old cache entries for HTTPS but IMO that wouldn't be so bad. What do you think?

Comment 49

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: CreateEncodedCertificate-example.patch

Here is an example of a test that uses CreateEncodedCertificate to create a test certificate. In the case of this test, we'd need to change CreateEncodedCertificate to take a SECItem* instead of a long as the serial number field.

Comment 50

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8418171 [details] [diff] [review]
CreateEncodedCertificate-example.patch

Wrong bug.

Comment 51

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #48)
> 1. The long-term solution to this problem and related ones is bug 660749. I
> understand that this is mostly wallpaper over bug 660749. Do you agree? If
> so, please add some comments to that effect so that we know.

Well, not entirely. The immediate issue is that we stopped doing network requests when querying nsNSSCertificate::hasValidEVOidTag(). With no network and no cached revocation information, we conclude any certificate is not EV.

> 2. TransportSecurityInfo::Write already has a versioning mechanism that we
> should be able to utilize here instead of inventing a new versoin detection
> scheme. What do you think?

I'll have a look.

> 3. Rather than trying to gracefully cope with missing serialized EV
> information in cached responses written by old versions, I think it is OK to
> simply fail to de-serialize them, as long as that causes the cache to reject
> (and doom) the entry and fall back to doing a network request. That is, I
> think it would be fine, for HTTPS cache entries only, to bump the version
> number written by TransportSecurityInfo::Write and then require that version
> in TransportSecurityInfo::Read. This would have the effect of invalidating
> all old cache entries for HTTPS but IMO that wouldn't be so bad. What do you
> think?

It would be great if that worked - in my experience investigating this, whenever a cache entry fails to deserialize, it doesn't get removed (and deserialization repeatedly fails). It also appears to be the case that new entries don't replace it. I'll take another look to see if I'm doing something wrong.

Comment 52

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8417699 [details] [diff] [review]
patch

Clearing review while I investigate further.

Comment 53

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) from comment #51)
> > in TransportSecurityInfo::Read. This would have the effect of invalidating
> > all old cache entries for HTTPS but IMO that wouldn't be so bad. What do you
> > think?
> 
> It would be great if that worked - in my experience investigating this,
> whenever a cache entry fails to deserialize, it doesn't get removed (and
> deserialization repeatedly fails). It also appears to be the case that new
> entries don't replace it. I'll take another look to see if I'm doing
> something wrong.

I see. If that's the behavior you are seeing then that's a bug in the HTTP cache. Failure to deserialize the SecurityInfo should result in us throwing away the cache entry.

Comment 54

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #53)
> (In reply to David Keeler (:keeler) from comment #51)
> > > in TransportSecurityInfo::Read. This would have the effect of invalidating
> > > all old cache entries for HTTPS but IMO that wouldn't be so bad. What do you
> > > think?
> > 
> > It would be great if that worked - in my experience investigating this,
> > whenever a cache entry fails to deserialize, it doesn't get removed (and
> > deserialization repeatedly fails). It also appears to be the case that new
> > entries don't replace it. I'll take another look to see if I'm doing
> > something wrong.
> 
> I see. If that's the behavior you are seeing then that's a bug in the HTTP
> cache. Failure to deserialize the SecurityInfo should result in us throwing
> away the cache entry.

The old cache back-end does it.  The new one does not.

Comment 55

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #54)
> The old cache back-end does it.  The new one does not.

Err... the check is now in nsHttpChannel::OpenCacheInputStream.  So it works for the new cache as well.

Comment 56

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v2

I think this will do it.

Comment 57

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8421964 [details] [diff] [review]
patch v2

Review of attachment 8421964 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +446,5 @@
>    
>    mSSLStatus = reinterpret_cast<nsSSLStatus*>(obj.get());
>  
>    if (!mSSLStatus) {
> +    return NS_ERROR_FAILURE;

Please explain how this works. How do you construct the nsSSLStatus after deserializing an TransportSecurityInfo that doesn't have one? I think some extensions and probably some parts of Gecko/Firefox/Thunderbird are expecting to have a valid SSLStatus for any valid document of https:// origin.

Comment 58

[Dana Keeler (she/her) (use needinfo) [:keeler]]

My understanding is if deserialization fails, the whole cache entry will be discarded, so there won't ever be a TransportSecurityInfo that doesn't have an mSSLStatus. Note that previously, deserialization would attempt to continue if reading the nsSSLStatus failed.

Comment 59

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) from comment #58)
> My understanding is if deserialization fails, the whole cache entry will be
> discarded, so there won't ever be a TransportSecurityInfo that doesn't have
> an mSSLStatus. Note that previously, deserialization would attempt to
> continue if reading the nsSSLStatus failed.

Got it & I agree that this is a good change.

Comment 60

[Honza Bambas (:mayhemer)]

David, I need some summary what this patch does and why, not easy to find in the bug.  It's hard to review otherwise.

Comment 61

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Sure thing. Basically, since bug 950240, we make sure not to do network requests when querying the EV status of a certificate/SSLStatus. However, when loading a page from the cache, this means the EV indicator is never shown. To get the best of both worlds, this patch caches the EV status along with the other SSLStatus information. However, since the format of the cache entry has changed, we need to be sure that deserialization of a TransportSecurityInfo will fail (and cause that entry to be thrown away) if the SSLStatus can't be deserialized.
Let me know if you need more details. I can also include an explanation as a comment in the patch.

Comment 62

[Honza Bambas (:mayhemer)]

Comment on attachment 8421964 [details] [diff] [review]
patch v2

Review of attachment 8421964 [details] [diff] [review]:
-----------------------------------------------------------------

TransportSecurityInfo::Read and Write support version.  This apparently changes the version format.  

If you want to persist more data, you need to change the version.  We are currently at version 2.  Yours will be 3.  You can easily fail the read when the version is not 3 to get your behavior - throw the cache entry on sec info deser failure.  I think you need to add the new value at the end of the stream data.  It means you need to expose the cached ev status from the cert.  Unfortunatelly, this will leave the data unread when v3 entries are read with v2 code (when people return to older version of Fx) since I've made the read logic unfortunately "forward compatible" :/  

Other option would be to add version to the certificate.  But I don't know how to do it in a compatible way.  Our sec info persistence is such a shit!  Maybe you could write the cached ev status between the length and the DER data so that the DER parser in the old code fails when it reads the cert back?

Comment 63

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the review. On a related note, do we have data that suggests it's worthwhile to support the behavior of having these cache entries be forwards and backwards compatible? It seems like a lot of pain to go through if it's not something users need.

Comment 64

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v3

Over email, I got the impression that while forwards/backwards compatibility would be nice, it isn't crucial. Indeed, it seems that supporting this has resulted in an unmaintainable mess, and I think it's more important that we have maintainable code. So, this patch essentially deliberately breaks the forwards/backwards compatibility in a way that shouldn't leave either past or future versions in a state they can't recover from.

Comment 65

[Lukas Blakk [:lsblakk] use ?needinfo]

Flagging Honza since this needs to be reviewed and nominated for uplift before next Monday if it's to get into FF30.

Comment 66

[Honza Bambas (:mayhemer)]

And what is the question please?

Comment 67

[Honza Bambas (:mayhemer)]

Review in progress.  r? flag is enough to catch my attention.  If you need superurgent review ping me on IRC or send a out-of-bound "URGENT" email and I'll gladly put you upper in my queue.  But empty ni?'s don't tell me much.

Comment 68

[Honza Bambas (:mayhemer)]

Comment on attachment 8426440 [details] [diff] [review]
patch v3

Review of attachment 8426440 [details] [diff] [review]:
-----------------------------------------------------------------

The patch doesn't apply cleanly on the current m-c.  After -F 6 patching it doesn't build...

Comment 69

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Honza Bambas (:mayhemer) from comment #67)
> Review in progress.  r? flag is enough to catch my attention.  If you need
> superurgent review ping me on IRC or send a out-of-bound "URGENT" email and
> I'll gladly put you upper in my queue.  But empty ni?'s don't tell me much.

My intention was to make sure you were aware we need this to be ready for uplift by Monday if it is to get into FF30. I will ping on IRC or email next time.

Comment 70

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v3.1

Sorry, Honza. This should apply to the latest m-c. Let me know if there are still compile issues (there aren't on my local machine).

Comment 71

[Honza Bambas (:mayhemer)]

(In reply to Lukas Blakk [:lsblakk] from comment #69)
> My intention was to make sure you were aware we need this to be ready for
> uplift by Monday if it is to get into FF30. I will ping on IRC or email next
> time.

Or just write what you want to the ni? comment :)  I see this more and more - ni? but it's not clear what is the one asking for...

Comment 72

[Honza Bambas (:mayhemer)]

(In reply to David Keeler (:keeler) [needinfo? is a good way to get my attention] from comment #70)
> Created attachment 8427966 [details] [diff] [review]
> patch v3.1
> 
> Sorry, Honza. This should apply to the latest m-c. Let me know if there are
> still compile issues (there aren't on my local machine).

ac_add_options --enable-warnings-as-errors ?

Comment 73

[Honza Bambas (:mayhemer)]

Comment on attachment 8426440 [details] [diff] [review]
patch v3

Review of attachment 8426440 [details] [diff] [review]:
-----------------------------------------------------------------

Tested locally, fantastic solution!  I really like it!

Please fix the build warnings:
Warning	3	warning C4018: '>' : signed/unsigned mismatch	c:\Mozilla\src\mozilla-central\security\manager\ssl\src\TransportSecurityInfo.cpp	359
Warning	5	warning C4018: '>' : signed/unsigned mismatch	c:\Mozilla\src\mozilla-central\security\manager\ssl\src\TransportSecurityInfo.cpp	368

Comment 74

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch v3.2

Thanks, Honza! Addressed compiler warnings, carrying over r+.
Try: https://tbpl.mozilla.org/?tree=Try&rev=e5391e05030c

Comment 75

[Lukas Blakk [:lsblakk] use ?needinfo]

With no branch patch ready and no one in IRC to check with at this time, we're missing the last Beta 30 build this was safe to take in.  Wontfixing (again) and we'll have to target FF31.

Comment 76

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Try looked good, so now that inbound is open: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5da62e82faf

Comment 77

[Ryan VanderMeulen [:RyanVM]]

Backed out for test_browserElement_oop_SecurityChange.html failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba1d7518428

https://tbpl.mozilla.org/php/getParsedLog.php?id=40470235&tree=Mozilla-Inbound

Comment 78

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bustage fix

Turns out, the serialization of nsNSSCertificateFakeTransport has to match that of nsNSSCertificate.
Here's a more thorough try run: https://tbpl.mozilla.org/?tree=Try&rev=5dfa41224603

Comment 79

[Honza Bambas (:mayhemer)]

Comment on attachment 8429463 [details] [diff] [review]
bustage fix

Review of attachment 8429463 [details] [diff] [review]:
-----------------------------------------------------------------

Ups, that escaped to me.  Thanks!

Comment 80

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the reviews!
The failures in the try run showed up for me on a vanilla mozilla-central checkout when I did an opt build on windows 7, so I believe they're unrelated to these changes. See bug 1005696.
I squashed the two patches together since they really should be one:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e97a6b080e

Comment 81

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/68e97a6b080e

Comment 82

[Sylvestre Ledru [:Sylvestre]]

David, once you are happy with your change in nightly. Could you fill an uplift request for 31? Thanks

Comment 83

[Dana Keeler (she/her) (use needinfo) [:keeler]]

This requires the patch in bug 1017636, so I've requested uplift for that first, then I'll do this.

Comment 84

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 950240
User impact if declined: no EV indicator when visiting a cached site
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): non-negligible (there was already one bug this patch uncovered, but it was fixed and uplifted)
String or IDL/UUID changes made by this patch: none

Comment 85

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3253bb6f2a28

Comment 86

[Paul Silaghi, QA [:pauly]]

addons.mozilla.org
bugzilla.mozilla.org
op.fi - all look ok
Verified fixed FF 32.0a2 (2014-06-13), Win 7 x64

Comment 87

[Henrik Skupin [:whimboo][⌚️UTC+2]]

David, any chance we can get some automated tests for this severe regression? If it's not possible, we might be able to create some for Mozmill.

Comment 88

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I'm not even sure it's possible to test something like this with our current testing infrastructure. Mozmill would be best.

Comment 89

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, I filed bug 1025849 for the Mozmill test. We will have it soon.

Comment 90

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed on Firefox 31 beta 5, build ID: 20140626181429 using the STR from the description.
Tested on Windows 7 64bit, Ubuntu 13.04 and Mac OS X 10.9.3.

User Agents:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Firefox/31.0

Comment 91

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Cornel Ionce [QA] from comment #90)
> Verified fixed on Firefox 31 beta 5, build ID: 20140626181429 using the STR
> from the description.

You are not testing with Nightly but with Beta. So you cannot mark this bug as verified fixed. You can only update the status flag as you did. For a full verification you will also have to verify the current Aurora build (Firefox 32.0a2).

Comment 92

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Aurora seems to have been already verified in comment 86. Since target milestone was 32, and this was in fact verified on both 31 and 32 (28, 29, 30 = wontfix) I think this should be marked as Verified.

Comment 93

[Mihaela Velimiroviciu (:mihaelav)]

This is reproducible again with the latest Nightly build:
Mozilla/5.0 (X11; Linux i686; rv:33.0) Gecko/20100101 Firefox/33.0, build id: 20140717030339,
Built from https://hg.mozilla.org/mozilla-central/rev/a74600665875

Steps to reproduce: the same as in bug description

Comment 94

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please do not reopen already verified and fixed bugs. If there is a new regression it warrants its own bug. So please file a new one and do a regression check. Thanks.

Comment 95

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Removing in-qa-testsuite flag given that we take care of this test in bug 1025849.