Closed Bug 995801 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Version:
29 Branch
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla32
Tracking Flags:
Tracking Status
firefox28 --- wontfix
firefox29 + wontfix
firefox30 + wontfix
firefox31 + verified
firefox32 + verified
relnote-firefox --- 29+

People

(Reporter: alice0775, Assigned: keeler)

References

Details

(Keywords: regression)

Attachments

(3 files, 5 obsolete files)

patch v3.2
11.77 KB, patch
keeler
: review+
Details | Diff | Splinter Review
bustage fix
4.75 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
patch for aurora
15.55 KB, patch
keeler
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
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"
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)
Blocks: 985623
Component: Security → DOM: Security
Summary: EV identifier is no longer displayed after doing "Restore Previous Session" → EV identifier is no longer displayed in Location Bar/Arrow Panel after doing "Restore Previous Session"
As far as I know, EV has nothing to do with safebrowsing lookups. This is very puzzling.
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.
Background tab also affected for me.
Anyway, when there is some such kind bug, I cannot believe Firefox with safety anymore.
The interesting fact is that when you switch between different affected versions, it will work again until the next restart.
I'm seeing this even in fa098f9fe89c so I suspect the bisection was erronous.
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.
(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.
Keywords: regressionwindow-wanted
A quick spot-check showed me that this started to fail sometime between 02/15 - 03/01.
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.
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
Blocks: 950240
status-firefox28: unaffected → affected
Version: 29 Branch → 28 Branch
hah, Firefox is completely broken....
The first bad revision is:
changeset:   170796:9d58d9a2c8b1
user:        Monica Chew <mmc@mozilla.com>
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!
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.
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.
(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
(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.
The hotfix won't have been applied if Alice was running Firefox != 27, 28 at the time we pushed it.
(FYI, I usually use Firefox24esr. I use release/beta/Aurora/Nightly for testing purpose (i.e. very short term))
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
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)
>(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.
Sorry. I can not believe.
Beta 9 go to build is tomorrow (at about 13 PDT), what do we do regarding this bug?
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?
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.
OK so it sounds like we will wontfix this for 29 as well given the timeline, based on Monica and David's comments.
status-firefox29: affected → wontfix
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.
Keywords: regressionwindow-wanted
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.
Summary: EV identifier is no longer displayed in Location Bar/Arrow Panel after doing "Restore Previous Session" → EV identifier is no longer displayed in Location Bar/Arrow Panel
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?
Flags: needinfo?(lsblakk)
Keywords: relnote
QA Contact: mwobensmith
Hardware: x86_64 → All
Summary: EV identifier is no longer displayed in Location Bar/Arrow Panel → EV identifier is no longer displayed in Location Bar/Arrow Panel after a restart of Firefox
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.
Version: 28 Branch → 29 Branch
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.
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.
>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.
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.
relnote-firefox: --- → ?
Flags: needinfo?(lsblakk) → needinfo?(sledru)
Will be part of the Known issues release notes for 29.
relnote-firefox: ? → 29+
Flags: needinfo?(sledru)
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.
David, any chance to see that bug fixed for 30? Thanks
Flags: needinfo?(dkeeler)
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".
If we end up doing a 29.0.1 release can we consider this for inclusion?
Yes, we will do a backout of bug 9504240 in order to restore this functionality in the non-urgent 29.0.1
Attached patch patch (obsolete) — — Splinter Review 
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.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8417699 - Flags: review?(brian)
Flags: needinfo?(dkeeler)
(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
>going the other way doesn't work so well.

Is updating the name of the cache file an option?
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?
Flags: needinfo?(dkeeler)
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 on attachment 8418171 [details] [diff] [review]
CreateEncodedCertificate-example.patch

Wrong bug.
Attachment #8418171 - Attachment is obsolete: true
(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.
Flags: needinfo?(dkeeler)
Comment on attachment 8417699 [details] [diff] [review]
patch

Clearing review while I investigate further.
Attachment #8417699 - Flags: review?(brian)
(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.
(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.
(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.
Keywords: relnote
Attached patch patch v2 (obsolete) — — Splinter Review 
I think this will do it.
Attachment #8417699 - Attachment is obsolete: true
Attachment #8421964 - Flags: review?(honzab.moz)
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.
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.
(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.
David, I need some summary what this patch does and why, not easy to find in the bug.  It's hard to review otherwise.
Flags: needinfo?(dkeeler)
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.
Flags: needinfo?(dkeeler)
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?
Attachment #8421964 - Flags: review?(honzab.moz) → review-
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.
Attached patch patch v3 (obsolete) — — Splinter Review 
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.
Attachment #8421964 - Attachment is obsolete: true
Attachment #8426440 - Flags: review?(honzab.moz)
Flagging Honza since this needs to be reviewed and nominated for uplift before next Monday if it's to get into FF30.
Flags: needinfo?(honzab.moz)
And what is the question please?
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.
Flags: needinfo?(honzab.moz)
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...
(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.
Attached patch patch v3.1 (obsolete) — — Splinter Review 
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).
Attachment #8426440 - Attachment is obsolete: true
Attachment #8426440 - Flags: review?(honzab.moz)
Attachment #8427966 - Flags: review?(honzab.moz)
(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...
(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 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
Attachment #8426440 - Attachment is obsolete: false
Attachment #8426440 - Attachment is obsolete: true
Attachment #8427966 - Flags: review?(honzab.moz) → review+
Attached patch patch v3.2 — — Splinter Review 
Thanks, Honza! Addressed compiler warnings, carrying over r+.
Try: https://tbpl.mozilla.org/?tree=Try&rev=e5391e05030c
Attachment #8427966 - Attachment is obsolete: true
Attachment #8428027 - Flags: review+
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.
status-firefox30: affected → wontfix
status-firefox32: --- → affected
Attached patch bustage fix — — Splinter Review 
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
Attachment #8429463 - Flags: review?(honzab.moz)
Comment on attachment 8429463 [details] [diff] [review]
bustage fix

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

Ups, that escaped to me.  Thanks!
Attachment #8429463 - Flags: review?(honzab.moz) → review+
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
https://hg.mozilla.org/mozilla-central/rev/68e97a6b080e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1017636
David, once you are happy with your change in nightly. Could you fill an uplift request for 31? Thanks
tracking-firefox32: ? → +
Flags: needinfo?(dkeeler)
This requires the patch in bug 1017636, so I've requested uplift for that first, then I'll do this.
Flags: needinfo?(dkeeler)
Attached patch patch for aurora — — Splinter Review 
[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
Attachment #8433496 - Flags: review+
Attachment #8433496 - Flags: approval-mozilla-aurora?
Attachment #8433496 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
addons.mozilla.org
bugzilla.mozilla.org
op.fi - all look ok
Verified fixed FF 32.0a2 (2014-06-13), Win 7 x64
status-firefox32: fixed → verified
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.
Flags: needinfo?(dkeeler)
Flags: in-testsuite?
I'm not even sure it's possible to test something like this with our current testing infrastructure. Mozmill would be best.
Flags: needinfo?(dkeeler)
Ok, I filed bug 1025849 for the Mozmill test. We will have it soon.
Flags: in-testsuite?
Flags: in-testsuite-
Flags: in-qa-testsuite?
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
Status: RESOLVED → VERIFIED
status-firefox31: fixed → verified
Keywords: verifyme
(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).
Status: VERIFIED → RESOLVED
Closed: 10 years ago → 10 years ago
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.
Status: RESOLVED → VERIFIED
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
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
No longer blocks: 1040086
Removing in-qa-testsuite flag given that we take care of this test in bug 1025849.
Flags: in-qa-testsuite?
Depends on: 1110690
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: