1528498 - Crash in [@ nsContentSecurityManager::IsOriginPotentiallyTrustworthy] via nsContentUtils::HttpsStateIsModern

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Wayne Mery (:wsmwk)]

#32 crash for 60.5.0, so topicrash. Not a new signature in version 60 - not sure how I missed filing a bug sooner. (crash stats says the signature is around since 2016)

bp-87bbed40-4a9a-49b1-a1d8-291d20190112 is a typical stack. Reporter writes "cannot be opened, then very slow, persistant flickering, this is happening to me quite ofter and can result in being down for up to 2 days."

Top 10 frames of crashing thread:

0 xul.dll nsContentSecurityManager::IsOriginPotentiallyTrustworthy dom/security/nsContentSecurityManager.cpp:826
1 xul.dll nsContentUtils::HttpsStateIsModern dom/base/nsContentUtils.cpp:10067
2 xul.dll nsGlobalWindowOuter::ComputeIsSecureContext dom/base/nsGlobalWindowOuter.cpp:1498
3 xul.dll nsGlobalWindowOuter::SetNewDocument dom/base/nsGlobalWindowOuter.cpp:1844
4 xul.dll nsPresContext::GetDocumentColorPreferences layout/base/nsPresContext.cpp:576
5 xul.dll nsDocumentViewer::InitInternal layout/base/nsDocumentViewer.cpp:932
6 xul.dll nsDocumentViewer::Init layout/base/nsDocumentViewer.cpp:666
7 xul.dll nsDocShell::SetupNewViewer docshell/base/nsDocShell.cpp:9128
8 xul.dll nsDocShell::Embed docshell/base/nsDocShell.cpp:6935
9 xul.dll nsDocShell::CreateContentViewer docshell/base/nsDocShell.cpp:8928

bp-25eec6d1-1945-4db8-814f-4b61a0190124 "opening takes a long time and I often get a question first if I allow Thunderbird to make changes to this computer" " openen duurt lang en ik krijg vaak eerst een vraag of ik toesta dat Thunderbird wijzigingen mag aanbrengen in deze computer"

Another user equates the crash with compact prompt.

Comment 1

[Wayne Mery (:wsmwk)]

(forgot to metion)
99.8% windows crashes. Only one or two other OS crashes, and I didn't find different signature for other OS that has a similar stack. So perhaps Windows AV related?

Comment 2

[Wayne Mery (:wsmwk)]

Magnus, is the stack useful?

In crash stats, no nightly crashes and seems it starts at TB60.0b10/60.0b11 (I suspect the crashes prior to this are outliers), with plenty of crashes, but then is gone from our betas. N.B. we did not ship beta 61 and beta 62, so potentially stopped in non-release builds between beta 60 and beta 63. Maybe got patched or code removed but not uplifted to ESR?

Comment 3

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/releases/mozilla-esr60/annotate/c23331af8ac635f86d748c0eb1e5bbb8fcfd86d9/dom/security/nsContentSecurityManager.cpp#l826

uri is null (got a few rows above). This is sill true on trunk, but some other condition must have changed to make it never happen.

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: bug1528498_content_sec_man_crash.patch

Add rv check

Comment 5

[Jorg K (CEST = GMT+2)]

Hmm, Valentin is a network peer, you'd better find a DOM security peer here, like Chris or Andrea (https://wiki.mozilla.org/Modules/Core#Content_Security).

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 9046222 [details] [diff] [review]
bug1528498_content_sec_man_crash.patch

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

Stealing review from baku - thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53992f6c29b3
check principal->GetURI rv to prevent Crash in [@ nsContentSecurityManager::IsOriginPotentiallyTrustworthy] via nsContentUtils::HttpsStateIsModern. r=ckerschb

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/53992f6c29b3

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is this worth a backport to Beta? It grafts cleanly, so go ahead and nominate if the answer is yes.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9046222 [details] [diff] [review]
bug1528498_content_sec_man_crash.patch

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: It crashes, at least in Thunderbird.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Just a check of a return status that is good programming practise. If not checked, code runs on and crashes.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: See above, crashes are never welcome.
• User impact if declined:
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String or UUID changes made by this patch:

Comment 11

[Magnus Melin [:mkmelin]]

Jörg already added the approval reqs now. Yes we'd like it on ESR, since that's where it's mainly causing a problem.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9046222 [details] [diff] [review]
bug1528498_content_sec_man_crash.patch

We are getting towards the end of the beta 66 cycle at this point. 
A #35 crash doesn't seem so urgent; I see the patch is not super complicated but I think this can wait for 67. If you strongly disagree let me know.

Comment 13

[Jorg K (CEST = GMT+2)]

Hmm, today is the 27th of February and the current cycle runs for another three weeks. So personally I wouldn't call three weeks before the end of a seven week interval "getting towards the end".

The fix we're talking about here is adding a missing status check. In case it didn't crash before, that status check is a no-op, in case it did crash before, it's avoiding the crash. It's really no risk and it is enhancing error handling and code correctness. We're not talking about a huge change that turns in inner workings of Firefox upside down.

I will take the patch to the Thunderbird release branch on mozilla-esr60, so whatever you decide won't affect our ESR users. I could do something similar for our beta releases.

So take that as strong disagreement, but whatever you do, we can work around it.

Comment 14

[Jorg K (CEST = GMT+2)]

And as far as I can see, it crashed FF as well:

https://crash-stats.mozilla.com/search/?signature=%3DnsContentSecurityManager%3A%3AIsOriginPotentiallyTrustworthy&product=Firefox&date=%3E%3D2018-12-06T00%3A00%3A00.000Z&date=%3C2019-02-27T19%3A42%3A00.000Z&_facets=install_time&_facets=version&_facets=address&_facets=moz_crash_reason&_facets=reason&_facets=build_id&_facets=platform_pretty_version&_facets=signature&_facets=useragent_locale&_sort=-date&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/87f3db8be2059c8ba8543a0443802392bc6390d4 on THUNDERBIRD_60_VERBRANCH