1206842 - Improve code stability of SendSSLErrorReport, it may break close tabs

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[YF (Yang)]

Attachment: patch v1

STR:
Uncertain, I encounter it when closing tabs, a tab is left and become a ghost.


I see the error in Browser Console:
TypeError: docShell.failedChannel.securityInfo is null content.js:304:9


This patch reference to a very similar code.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8663818 [details] [diff] [review]
patch v1

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

Instead of try/catch I think it would be better to simply check |if (docShell.failedChannel.securityInfo) { ...|. Also because we need this check in two places, why not add a helper function that takes a docShell and returns its serialized SSL status? If securityInfo==null then it would simply return an empty string.

Comment 2

[YF (Yang)]

(In reply to Tim Taubert [:ttaubert] from comment #1)
> Comment on attachment 8663818 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8663818 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Instead of try/catch I think it would be better to simply check |if
> (docShell.failedChannel.securityInfo) { ...|. Also because we need this
> check in two places, why not add a helper function that takes a docShell and
> returns its serialized SSL status? If securityInfo==null then it would
> simply return an empty string.

I'm not sure why we was not do it, but maybe these QueryInterface and SSLStatus may be null too, so try to be more comprehensive.


And, it indeed no error dump, but the same try & catch is rampant in the file. so I did not add it alone, if necessary, it should probably be a separate bug.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to YF (Yang) from comment #2)
> And, it indeed no error dump, but the same try & catch is rampant in the
> file.

Well rampant is maybe not the right word, but yes there's another place doing that already. We can surely leave the try/catch but still, moving it to a helper function is probably good.

Comment 4

[YF (Yang)]

I don't have experience and interest to writing a new helper function at present :(

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1206842 - check we have a failedChannel and securityInfo before serializing, extract to helper, original patch by YF (Yang),

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

Comment 6

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8768838 [details]
Bug 1206842 - check we have a failedChannel and securityInfo before serializing, extract to helper, original patch by YF (Yang),

https://reviewboard.mozilla.org/r/62852/#review59966

lgtm

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0164fbe34683
check we have a failedChannel and securityInfo before serializing, extract to helper, original patch by YF (Yang), r=fkiefer

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0164fbe34683