Closed Bug 1475647 Opened 5 years ago Closed 5 years ago

Remove nsISSLStatusProvider interface

Categories

(Core :: Security: PSM, enhancement, P1)

Product:
Core
Component:
Security: PSM
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Future
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: dipen, Assigned: dipen)

References

Details

(Whiteboard: [psm-assigned][psm-contractor])

Attachments

(1 file, 1 obsolete file)

Bug 1475647 - Remove nsISSLStatusProvider interface.
59 bytes, text/x-review-board-request
jryans
: review+
mcmanus
: review+
keeler
: review+
Gijs
: review+
baku
: review+
Details 
As a first step in consolidating the nsISSLStatus into nsITransportSecurityInfo, remove the use of nsISSLStatusProvider.  Include nsISSLStatus as a member of classes that implemented nsISSLStatusProvider.
(In reply to Dipen Patel [:dipen] from comment #0)
> As a first step in consolidating the nsISSLStatus into
> nsITransportSecurityInfo, remove the use of nsISSLStatusProvider.  Include
> nsISSLStatus as a member of classes that implemented nsISSLStatusProvider.

:dipen, our practice is for the priority of the bug to be set by the triage owner of the component. Is :keeler in agreement with the priority you set for this? 

Thanks.
Flags: needinfo?(dkeeler)
Yes - :dipen is a contractor working on some improvements to PSM. Thanks!
Assignee: nobody → bugzilla
Blocks: 1468222
Flags: needinfo?(dkeeler)
Summary: Remove nsISSLTransportProvider interface → Remove nsISSLStatusProvider interface
Whiteboard: [psm-assigned][psm-contractor]
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review264182

Cool - this does touch a number of files, but the changes are minimal and mostly straightforward to verify that they're correct. The main issue is I would prefer we not add the additional `nsITransportSecurityInfoProvider` interface, since the goal here is to remove an interface. It looks like we should be able to do this by adding a `secInfo` attribute to `nsISecurityBrowserUI` (see the 3rd comment). I also noted a few style nits. Otherwise, looks good!

::: commit-message-fe17a:1
(Diff revision 1)
> +Bug 1475647 - Remove nsISSLStatusProvider interface. Access nsISSLStatus directly as a member of nsITransportSecurityInfo or through nsITransportSecurityInfoProvider. This is part of a larger effort to consolidte nsISSLStatus and nsITransportSecurityInfo.  r?keeler,gijs,pbro,mcmanus,sebastian

style nit: for our commit messages, usually the first line is a short description ending with the reviewers (<100 characters total) followed by a blank line and then as much explanatory text as necessary to describe the changes

::: security/manager/pki/resources/content/exceptionDialog.js:77
(Diff revision 1)
>   * @param {Event} evt
>   *        The load or error event.
>   */
>  function grabCert(req, evt) {
>    if (req.channel && req.channel.securityInfo) {
> -    gSSLStatus = req.channel.securityInfo
> +    gSSLStatus = req.channel.securityInfo.QueryInterface(Ci.nsITransportSecurityInfo).SSLStatus;

style nit: lines in this file are generally <80 characters (other js files wrap at 100), so let's have this look a bit more like the original, but with `nsITransportSecurityInfo`.

::: security/manager/ssl/nsITransportSecurityInfoProvider.idl:11
(Diff revision 1)
> +#include "nsISupports.idl"
> +
> +interface nsITransportSecurityInfo;
> +
> +[scriptable, uuid(fe68e888-dea6-4ec6-942c-fc12f8a61bdb)]
> +interface nsITransportSecurityInfoProvider : nsISupports {

I think we can avoid adding this interface by adding the `secInfo` attribute here to `nsISecureBrowserUI`, right? I think that would be preferable (given that our goal is to remove an interface).

::: security/manager/ssl/tests/unit/test_ssl_status.js:24
(Diff revision 1)
>    // Test successful connection (failedCertChain should be null,
>    // succeededCertChain should be set as expected)
>    add_connection_test(
>      "good.include-subdomains.pinning.example.com", PRErrorCodeSuccess, null,
> -    function withSecurityInfo(aSSLStatus) {
> -      let sslstatus = aSSLStatus.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus;
> +    function withSecurityInfo(aSecInfo) {
> +      let sslstatus = aSecInfo.QueryInterface(Ci.nsITransportSecurityInfo).SSLStatus;

`aSecInfo` should have already been QI'd to `nsITransportSecurityInfo`, so the `QueryInterface` here shouldn't be necessary.

::: security/manager/ssl/tests/unit/test_ssl_status.js:37
(Diff revision 1)
>    // Test failed connection (failedCertChain should be set as expected,
>    // succeededCertChain should be null)
>    add_connection_test(
>      "expired.example.com", SEC_ERROR_EXPIRED_CERTIFICATE, null,
> -    function withSecurityInfo(aSSLStatus) {
> -      let sslstatus = aSSLStatus.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus;
> +    function withSecurityInfo(aSecInfo) {
> +      let sslstatus = aSecInfo.QueryInterface(Ci.nsITransportSecurityInfo).SSLStatus;

Same here.

::: security/manager/tools/getHSTSPreloadList.js:114
(Diff revision 1)
>    let includeSubdomains = { value: false };
>    let error = ERROR_NONE;
>    if (header != null && securityInfo != null) {
>      try {
>        let uri = Services.io.newURI("https://" + host.name);
> -      let sslStatus = securityInfo.QueryInterface(Ci.nsISSLStatusProvider)
> +      let sslStatus = securityInfo.QueryInterface(Ci.nsITransportSecurityInfo).SSLStatus;

style nit: this line is a bit long
Attachment #8992461 - Flags: review?(dkeeler) → review-
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review264182

> I think we can avoid adding this interface by adding the `secInfo` attribute here to `nsISecureBrowserUI`, right? I think that would be preferable (given that our goal is to remove an interface).

Adding secInfo to nSISecureBrowserUI required updates to dom/ipc/TabParent.\* as it implements it nsISecureBrowserUI. Not sure why I shied away from this in favor of the provider interface.  Maybe because of unknown territory.

Taking a second look, it seems stratight forward to add secInfo attribute to nsISecureBrowserUI.  I would just have it return null in TabParent, similar to GetState.  Does that sound reasonable?
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review264182

> Adding secInfo to nSISecureBrowserUI required updates to dom/ipc/TabParent.\* as it implements it nsISecureBrowserUI. Not sure why I shied away from this in favor of the provider interface.  Maybe because of unknown territory.
> 
> Taking a second look, it seems stratight forward to add secInfo attribute to nsISecureBrowserUI.  I would just have it return null in TabParent, similar to GetState.  Does that sound reasonable?

Huh - well, it sure seems that if GetState returns null (well, 0, but essentially "nothing") then GetSecInfo returning null would be equally valid. So, yes, sounds good to me. I guess one question to ask a dom peer would be: "does TabParent need to implement nsISecureBrowserUI at all?" but that can be dealt with separately.
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review264182

> style nit: lines in this file are generally <80 characters (other js files wrap at 100), so let's have this look a bit more like the original, but with `nsITransportSecurityInfo`.

There are several lines exceeding this rule for this file.  I can udpate the others as well or we establish the new norm of 100.  Please let me know.
Attachment #8992461 - Flags: review?(pbrosset) → review?(jryans)
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review264388

the netwerk/* changes are fine
Attachment #8992461 - Flags: review?(mcmanus) → review+
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review264182

> There are several lines exceeding this rule for this file.  I can udpate the others as well or we establish the new norm of 100.  Please let me know.

I would say keep the existing lines unless there are only one or two or they're right next to what you're changing. Let's certainly avoid introducing new lines that are longer than 80 characters (fyi most of these style nits are from https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style )
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review264532

I believe you should only need to QI JS variable X to interface IX if the IDL definition of the interface you're using to obtain X doesn't already declare that property as being of type IX (or some IX' that inherits from IX). So I think most of the QIs you're adding are superfluous, especially if you're removing the nsITransportSecurityInfoProvider interface again. I didn't really go through all of this patch yet, I'll review again when you've updated for :keeler's feedback - but please try to avoid adding more QI calls unless they're actually necessary.

::: browser/base/content/pageinfo/security.js:53
(Diff revision 1)
> -    ui.QueryInterface(nsISSLStatusProvider);
> -    var status = ui.SSLStatus;
> +    ui.QueryInterface(nsITransportSecurityInfoProvider);
> +    var status = ui.secInfo.QueryInterface(Ci.nsITransportSecurityInfo).SSLStatus;

So my understanding from :keeler's review is that you'll remove this interface again, right? Because right now this has more xpcom goop than before, which is a bit sad... Do we really need the QI to the nsITransportSecurityInfo, even where we didn't before?

::: browser/base/content/pageinfo/security.js:57
(Diff revision 1)
>        (ui.state & Ci.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL);
> -    ui.QueryInterface(nsISSLStatusProvider);
> -    var status = ui.SSLStatus;
> +    ui.QueryInterface(nsITransportSecurityInfoProvider);
> +    var status = ui.secInfo.QueryInterface(Ci.nsITransportSecurityInfo).SSLStatus;
>  
>      if (!isInsecure && status) {
>        status.QueryInterface(nsISSLStatus);

If the interface definition of the SSLStatus property declares this as returning nsISSLStatus we really shouldn't need to QI here.

::: mobile/android/modules/geckoview/GeckoViewProgress.jsm:162
(Diff revision 1)
>        result.host = IDNService.convertToDisplayIDN(uri.host, {});
>      } catch (e) {
>        result.host = uri.host;
>      }
>  
> -    let status = aBrowser.securityUI.QueryInterface(Ci.nsISSLStatusProvider)
> +    this._lastStatus = aBrowser.securityUI;

I don't see you using `_lastStatus`? And why is it a different kind of object from `status`?

::: mobile/android/modules/geckoview/GeckoViewProgress.jsm:163
(Diff revision 1)
> +    let status = aBrowser.securityUI
> +                         .QueryInterface(Ci.nsITransportSecurityInfoProvider)
> +                         .secInfo.QueryInferface(Ci.nsITransportSecurityInfo)

You shouldn't need any of these QIs, I think.
Attachment #8992461 - Flags: review?(gijskruitbosch+bugs) → review-
(I don't see code that I recognize, so I removed my review request.)
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review264532

> So my understanding from :keeler's review is that you'll remove this interface again, right? Because right now this has more xpcom goop than before, which is a bit sad... Do we really need the QI to the nsITransportSecurityInfo, even where we didn't before?

Yes, all of :keeler's review comments have been applied and submitted for review.

> I don't see you using `_lastStatus`? And why is it a different kind of object from `status`?

This is indeed unnecessary.  Will be removed.
Comment on attachment 8992793 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257630/#review264568

I will delegate the netwerk portions of this to keeler
Attachment #8992793 - Flags: review?(mcmanus)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> (I don't see code that I recognize, so I removed my review request.)
I don't feel comfortable doing this review myself, and I saw your name as the main contributor of code in devtools/shared/security/auth.js and devtools/shared/security/socket.js which are both in the patch up for review now.
The rest seems to be in a webconsole-related folder, so maybe Nicolas or Honza or someone else could review that.
Sorry to insist, but do mind reviewing at least the 2 devtools/shared/security changes?
Flags: needinfo?(jryans)
Comment on attachment 8992793 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257630/#review264676

I stopped checking after a while, but I don't think any of the QIs to nsITransportSecurityInfo that remain are needed here.

Can you collapse/fold/roll this cset into the other one so there's just 1 review, please?

::: browser/base/content/pageinfo/security.js:52
(Diff revision 1)
>      var isInsecure =
>        (ui.state & Ci.nsIWebProgressListener.STATE_IS_INSECURE);
>      var isEV =
>        (ui.state & Ci.nsIWebProgressListener.STATE_IDENTITY_EV_TOPLEVEL);
> -    ui.QueryInterface(nsITransportSecurityInfoProvider);
>      var status = ui.secInfo.QueryInterface(Ci.nsITransportSecurityInfo).SSLStatus;

We don't QI the secInfo property elsewhere, is it actually necessary here?

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> -                               .QueryInterface(Ci.nsITransportSecurityInfoProvider)
> -                               .secInfo.QueryInferface(Ci.nsITransportSecurityInfo).SSLStatus;
> +                           .secInfo.QueryInferface(Ci.nsITransportSecurityInfo).SSLStatus;

Same here and in the Android code
Attachment #8992793 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review264684

Clearing review to get this out of my queue. I'll re-review a merged cset.
Attachment #8992461 - Flags: review?(gijskruitbosch+bugs)
Attachment #8992461 - Flags: review?(pbrosset)
Attachment #8992461 - Flags: review?(jryans)
Attachment #8992461 - Flags: review?(dkeeler)
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review264746

The /devtools changes here look fine to me.
Attachment #8992461 - Flags: review?(jryans) → review+
(In reply to Patrick Brosset <:pbro> from comment #16)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> > (I don't see code that I recognize, so I removed my review request.)
> I don't feel comfortable doing this review myself, and I saw your name as
> the main contributor of code in devtools/shared/security/auth.js and
> devtools/shared/security/socket.js which are both in the patch up for review
> now.
> The rest seems to be in a webconsole-related folder, so maybe Nicolas or
> Honza or someone else could review that.
> Sorry to insist, but do mind reviewing at least the 2
> devtools/shared/security changes?

Okay, I think I was confused because my review was requested on part 2, but only part 1 has /devtools changes.  Part 1 looks fine, I have marked in r+ for /devtools.
Flags: needinfo?(jryans)
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

I'm not the right person to review this. Forwarding to :jchen in the hope that he is or knows someone better.
Attachment #8992461 - Flags: review?(s.kaspari) → review?(nchen)
Attachment #8992793 - Flags: review?(s.kaspari) → review?(nchen)
Comment on attachment 8992793 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257630/#review264816

This looks good but I think it would be preferable to review and land these changes as one changeset rather than two.

::: dom/ipc/TabParent.cpp:898
(Diff revision 1)
> +TabParent::GetSecInfo(nsITransportSecurityInfo** _result)
> +{
> +  NS_ENSURE_ARG_POINTER(_result);
> +  NS_WARNING("TransportSecurityInfo not valid here");
> +  *_result = nullptr;
> +  NS_IF_ADDREF(*_result);

nit: this will always be null, so no need for the NS_IF_ADDREF
Attachment #8992793 - Flags: review?(dkeeler)
Attachment #8992793 - Attachment is obsolete: true
Attachment #8992793 - Flags: review?(nchen)
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review265026

Nice! Look at all that nice shiny code without xpcom goop... (also,  +124 / -146  is a nice diff stat for something like this!)

A few nits, and two more serious things in browser-siteIdentity.js and mobile/android/chrome/content/browser.js , but with those fixed I think this is good to go from the browser side (so with reviews from the others :-) ).

::: browser/base/content/browser-siteIdentity.js:350
(Diff revision 3)
> -    this._sslStatus = gBrowser.securityUI
> -                              .QueryInterface(Ci.nsISSLStatusProvider)
> -                              .SSLStatus;
> +    let secinfo = gBrowser.securityUI.secInfo;
> +    if (secinfo) {
> +      this._sslStatus = secinfo.SSLStatus;
> -    if (this._sslStatus) {
> -      this._sslStatus.QueryInterface(Ci.nsISSLStatus);
>      }

This will not overwrite the value if secinfo is null, which means we'll keep the old ssl status object around. That's almost certainly not good (ie we'll show the wrong ssl info for a tab / URL). Under what circumstances is secInfo null? I expect we should just set `_sslStatus` to null as well. Judging by the old code that used to be possible before.

::: browser/base/content/pageinfo/security.js:52
(Diff revision 3)
> -    ui.QueryInterface(nsISSLStatusProvider);
> -    var status = ui.SSLStatus;
> +    var status;
> +    if (ui.secInfo) {
> +      status = ui.secInfo.SSLStatus;
> +    }

Nit: `var status = ui.secInfo && ui.secInfo.SSLStatus;`

::: mobile/android/chrome/content/browser.js:5697
(Diff revision 3)
>    /**
>     * Determine the identity of the page being displayed by examining its SSL cert
>     * (if available). Return the data needed to update the UI.
>     */
>    checkIdentity: function checkIdentity(aState, aBrowser) {
> -    this._lastStatus = aBrowser.securityUI
> +    this._lastStatus = aBrowser.securityUI.secInfo.SSLStatus;

`secInfo` could be null, right? Or can that not happen on mobile? If so, we should make sure we set `_lastStatus` to null in that case.

::: mobile/android/modules/geckoview/GeckoViewProgress.jsm:162
(Diff revision 3)
> -    let status = aBrowser.securityUI.QueryInterface(Ci.nsISSLStatusProvider)
> +    let status = aBrowser.securityUI.secInfo.SSLStatus;
> -                         .SSLStatus.QueryInterface(Ci.nsISSLStatus);
>      let cert = status.serverCert;

Here and in content.js I think `secInfo` and/or the status object could be  null, but it could also be null pre-patch, and we didn't catch it, so could probably leave like this for now, but might be worth a follow-up bug?

::: toolkit/content/browser-child.js:217
(Diff revision 3)
>    onSecurityChange: function onSecurityChange(aWebProgress, aRequest, aState) {
>      let json = this._setupJSON(aWebProgress, aRequest);
>      let objects = this._setupObjects(aWebProgress, aRequest);
>  
>      json.state = aState;
> -    json.status = SecurityUI.getSSLStatusAsString();
> +    json.secinfo = SecurityUI.getSecInfoAsString();

Nit: please make this `secInfo` here and in toolkit/modules/RemoteWebProgress.jsm .

::: toolkit/content/browser-child.js:376
(Diff revision 3)
>  
>  WebNavigation.init();
>  
>  var SecurityUI = {
> -  getSSLStatusAsString() {
> -    let status = docShell.securityUI.QueryInterface(Ci.nsISSLStatusProvider).SSLStatus;
> +  getSecInfoAsString() {
> +    let secinfo = docShell.securityUI.secInfo;

Nit for my sanity :-)

if the property is `secInfo`, please make the local variable also `secInfo` and not `secinfo` :-)
Attachment #8992461 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review265204

Awesome! r=me with comments addressed and in particular a DOM peer saying that the TabParent changes are ok.

::: browser/base/content/browser-siteIdentity.js:350
(Diff revision 3)
>      this._state = state;
>  
>      // Firstly, populate the state properties required to display the UI. See
>      // the documentation of the individual properties for details.
>      this.setURI(uri);
> -    this._sslStatus = gBrowser.securityUI
> +    let secinfo = gBrowser.securityUI.secInfo;

Yeah, let's just s/secinfo/secInfo/ everywhere if the property is `secInfo`.

::: dom/ipc/TabParent.cpp:893
(Diff revision 3)
>    *aState = 0;
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +TabParent::GetSecInfo(nsITransportSecurityInfo** _result)

We should actually have a DOM peer sign off on this.

::: testing/marionette/puppeteer/firefox/firefox_puppeteer/api/security.py:46
(Diff revision 3)
>          :returns: Certificate details as JSON object.
>          """
>          cert = self.marionette.execute_script("""
>            var securityUI = arguments[0].linkedBrowser.securityUI;
> -          var status = securityUI.QueryInterface(Components.interfaces.nsISSLStatusProvider)
> +          var status = securityUI
> +                         .secInfo.QueryInterface(Components.interfaces.nsITransportSecurityInfo)

`secInfo` is already a `nsITransportSecurityInfo`, right? So this `QueryInterface` shouldn't be necessary.
Attachment #8992461 - Flags: review?(dkeeler) → review+
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review265026

> Here and in content.js I think `secInfo` and/or the status object could be  null, but it could also be null pre-patch, and we didn't catch it, so could probably leave like this for now, but might be worth a follow-up bug?

There is code several lines above to check the state `(aState & Ci.nsIWebProgressListener.STATE_IS_BROKEN)` and return appropriately.  However, I believe this check should also include checking if the stat is `STATE_IS_INSECURE` and return.  I created https://bugzilla.mozilla.org/show_bug.cgi?id=1477070
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review265632

::: security/manager/pki/resources/content/exceptionDialog.js:30
(Diff revision 4)
>    gSecHistogram = Services.telemetry.getHistogramById("SECURITY_UI");
>    gNsISecTel = Ci.nsISecurityUITelemetry;
>  
>    var brandName = gBundleBrand.getString("brandShortName");
> -  setText("warningText", gPKIBundle.getFormattedString("addExceptionBrandedWarning2", [brandName]));
> +  setText("warningText",
> +     gPKIBundle.getFormattedString("addExceptionBrandedWarning2", [brandName]));

indent gPKIBundle under "warningText"
Attachment #8992461 - Flags: review?(amarchesini) → review+
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review265204

> We should actually have a DOM peer sign off on this.

:baku was added to the review and signed off.
Attachment #8992461 - Flags: review?(s.kaspari) → review?(nchen)
Comment on attachment 8992461 [details]
Bug 1475647 - Remove nsISSLStatusProvider interface.

https://reviewboard.mozilla.org/r/257320/#review265786
Attachment #8992461 - Flags: review?(nchen) → review+
Keywords: checkin-needed
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d126a6593e8f
Remove nsISSLStatusProvider interface. r=baku,Gijs,jchen,jryans,keeler,mcmanus
Keywords: checkin-needed
Backed out changeset d126a6593e8f (bug 1475647) for mozmake.exe bustage on a CLOSED TREE

Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=189662314
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189662314&repo=autoland&lineNumber=18301

23:47:50     INFO -  2 warnings and 3 errors generated.
23:47:50     INFO -  z:/build/build/src/config/rules.mk:1052: recipe for target 'Unified_cpp_protocol_http2.i_o' failed
23:47:50     INFO -  mozmake.EXE[5]: *** [Unified_cpp_protocol_http2.i_o] Error 1
23:47:50     INFO -  mozmake.EXE[5]: Leaving directory 'z:/build/build/src/obj-firefox/netwerk/protocol/http'
23:47:50     INFO -  mozmake.EXE[5]: *** Waiting for unfinished jobs....
Flags: needinfo?(bugzilla)
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/87c7f405385f
Backed out changeset d126a6593e8f for mozmake.exe bustage on a CLOSED TREE
The build failed because of a missing include file for Windows platform.  This will be fixed, verified and re-submitted.
Flags: needinfo?(bugzilla)
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c235d6f86c22
Remove nsISSLStatusProvider interface. r=baku,Gijs,jchen,jryans,keeler,mcmanus
Keywords: checkin-needed
Fixed test failures

testing/firefox-ui/tests/puppeteer/test_tabbar.py
testing/firefox-ui/tests/puppeteer/test_security.py

Expanded the subsequent try run to include more tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58fff77cb67ca10c871a55ab2098bb65f0b31aa4&group_state=expanded
Flags: needinfo?(bugzilla)
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46f641da7f89
Remove nsISSLStatusProvider interface. r=baku,Gijs,jchen,jryans,keeler,mcmanus
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/46f641da7f89
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.