Port bug 1468222: Consolidate nsISSLStatus into nsITransportSecurityInfo (cannot find type `nsISSLStatus` in this scope)

RESOLVED FIXED in Thunderbird 64.0

Status

defect
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 64.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

M-C last good: fea371cafd2c73be689184ca6670e33c3f
M-C first bad: 423bdf7a802b0d302244492b423609187d

Range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fea371cafd2c73be689184ca6670e33c3f&tochange=423bdf7a802b0d302244492b423609187d

0:58.41 dom/media/platforms/ffmpeg/ffvpx
0:59.74 dom/media/systemservices
1:00.05 cannot find type `nsISSLStatus` in this scope
1:00.05 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/xpcrs/rt/imIAccount.rs:773:101
1:00.05     pub GetSslStatus: unsafe extern "system" fn (this: *const prplIAccount, aSslStatus: *mut *const nsISSLStatus) -> nsresult,
1:00.05  cannot find type `nsISSLStatus` in this scope
1:00.05 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/xpcrs/rt/imIAccount.rs:1053:63
1:00.05     pub unsafe fn GetSslStatus(&self, aSslStatus: *mut *const nsISSLStatus) -> nsresult {
1:00.05                                       ^^^^^^^^^^^^not found in this scope
1:02.94 dom/media/wave
That comes from chat's imIAccount.idl which is somehow processed into Rust and there's something wrong.

We have:
  interface nsISSLStatus;
and
  readonly attribute nsISSLStatus sslStatus;

I see it now:
5cfda4227c6a Dipen Patel - Bug 1468222 Consolidate nsISSLStatus info nsITransportSecurityInfo r=Gijs,snorp,jcj,mcmanus,sfraser,keeler,baku,ato
Summary: Thunderbird builds busted: 2018-09-11 by rust compile error: cannot find type `nsISSLStatus` in this scope → Port bug 1468222: Consolidate nsISSLStatus into nsITransportSecurityInfo (cannot find type `nsISSLStatus` in this scope)
Looks like we need to look at the call sites providing SSL status:
https://searchfox.org/comm-central/search?q=sslStatus&case=false&regexp=false&path=
This lets us compile again.
Magnus, would you like to be the doctor here? The paramedics morning shift is over and the patient is no longer bleeding.
Flags: needinfo?(mkmelin+mozilla)
Keywords: leave-open
Make that genitive: The paramedic's ...
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/11615766cf8a
Port bug 1468222: remove use of nsISSLStatus. rs=bustage-fix
Hi Dipen, anything in particular we need to look for when porting this? Just removing the interface (see comment #6) hasn't caused any test failures, but of course not everything is covered by tests.
Flags: needinfo?(bugzilla)
Can take a look in the evening, if nobody else wants to do it.
Assignee: nobody → mkmelin+mozilla
Flags: needinfo?(mkmelin+mozilla)
OK, DXR is back :-)
https://dxr.mozilla.org/comm-central/search?q=sslStatus+path%3Acomm%2F&redirect=false

https://dxr.mozilla.org/comm-central/search?q=nsISSLStatus+path%3Acomm%2F&redirect=false
Mostly comments and the stuff I've already removed in https://hg.mozilla.org/comm-central/rev/11615766cf8a, but
comm/mail/components/accountcreation/content/guessConfig.js
859 let cert = status.QueryInterface(Ci.nsISSLStatus).serverCert;
surely won't work now.

Replacement is here:
https://hg.mozilla.org/mozilla-central/rev/5cfda4227c6a#l10.12
Version: 63 → Trunk
Posted patch 1490265-nsISSLStatus.patch (obsolete) — Splinter Review
I've made a start gleaning bits from
https://hg.mozilla.org/mozilla-central/rev/5cfda4227c6a
That should cover everything except chat/modules/jsProtoHelper.jsm.

Your turn, Magnus?
Attachment #9008149 - Attachment is obsolete: true
Ah well, this should do it.
Assignee: mkmelin+mozilla → jorgk
Attachment #9008153 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(bugzilla)
Attachment #9008156 - Flags: review?(mkmelin+mozilla)
Attachment #9008156 - Flags: feedback?(bugzilla)
Comment on attachment 9008156 [details] [diff] [review]
1490265-nsISSLStatus.patch (v3)

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

::: chat/modules/jsProtoHelper.jsm
@@ +59,5 @@
>  
>      if (aIsSslError)
>        return Ci.prplIAccount.ERROR_ENCRYPTION_ERROR;
>  
> +    let secInfo = this._secInfo = aSocket.secInfo;

Actually, I didn't see an example of this in the M-C patch.
Comment on attachment 9008156 [details] [diff] [review]
1490265-nsISSLStatus.patch (v3)

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

Looks ok to me. It would be a bit complicated to test though. r=mkmelin

::: chat/components/public/imIAccount.idl
@@ +149,5 @@
>    /* When a certificate error occurs, the host/port that caused a
>     * SSL/certificate error when connecting to it. This is only valid when
>     * connectionErrorReason is one of ERROR_CERT_*. */
>    readonly attribute AUTF8String connectionTarget;
> +  /* When a certificate error occurs, the nsITransportSecurityInfo error of the socket. This

comment style is wrong in this file

/** is documentation style comments. /* is just start of a multiline comment
Attachment #9008156 - Flags: review?(mkmelin+mozilla) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/37bdf33fc10c
Port bug 1468222: Replace nsISSLStatus with nsITransportSecurityInfo. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Comment on attachment 9008156 [details] [diff] [review]
1490265-nsISSLStatus.patch (v3)

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

A little late but looks fine.
Attachment #9008156 - Flags: feedback?(bugzilla) → feedback+
Better late than never, as they say ;-) - Thanks!
You need to log in before you can comment on or make changes to this bug.