1649728 - Crash in [@ mozilla::SharedStyleSheetCache::CancelLoadsForLoader]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[[:philipp]]

This bug is for crash report bp-ab4b473d-2a8a-427a-bb3d-533c00200701.

Top 10 frames of crashing thread:

0 xul.dll mozilla::SharedStyleSheetCache::CancelLoadsForLoader layout/style/SharedStyleSheetCache.cpp:503
1 xul.dll mozilla::css::Loader::DeregisterFromSheetCache layout/style/Loader.cpp:523
2 xul.dll mozilla::dom::Document::SetPrincipals dom/base/Document.cpp:3748
3 xul.dll mozilla::dom::Document::ResetToURI dom/base/Document.cpp:2539
4 xul.dll mozilla::dom::Document::Reset dom/base/Document.cpp:2445
5 xul.dll mozilla::dom::Document::StartDocumentLoad dom/base/Document.cpp:3064
6 xul.dll mozilla::dom::XMLDocument::StartDocumentLoad dom/xml/XMLDocument.cpp:251
7 xul.dll nsContentDLF::CreateInstance layout/build/nsContentDLF.cpp
8 xul.dll mozilla::image::SVGDocumentWrapper::SetupViewer image/SVGDocumentWrapper.cpp:292
9 xul.dll mozilla::image::SVGDocumentWrapper::OnStartRequest image/SVGDocumentWrapper.cpp:196

this is another content crash signature looking related to the changes from bug 1599160 landing in 79. different to bug 1646330 it's also hitting users on the beta channel though.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I think this is an instance of bug 1605895. We shouldn't have a bogus data there unless necko doesn't call us back to clean up, which is what bug 1605895 is about :/

I could probably paper over this somehow, but it'd probably leak instead of crashing, which is not amazing either...

Comment 2

[Sean Voisen (:svoisen)]

Assigning to Emilio for now just to keep an eye on until bug 1605895 is closed.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Bug 1649555 has STR. Will take a look if Honza can't figure out with that. This is a pre-existing issue, but it seems way more prominent because we now use the cache across documents, so we should try to get this fixed in 79.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1649728 - Use WeakPtr in SharedStyleSheetCache::mLoadingDatas. r=heycam,#style

This is safer in case Necko fails to notify us. The only repro we have
is fixed by bug 1651661, but this should hopefully be uncontroversial as
well and prevents crashing in release builds.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

The only repro we have is bug 1651661, but I think landing this defensive patch is worth it.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9163770 [details]
Bug 1649728 - Use WeakPtr in SharedStyleSheetCache::mLoadingDatas. r=heycam,#style

Beta/Release Uplift Approval Request

• User impact if declined: Crashes.
• 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): Very minor change to not store raw pointers, and store WeakPtr<>s instead, so that we get null instead of garbage when Necko fails to notify (and thus we fail to clean up).

This doesn't fix the correctness issue (there are <link> elements which won't get their load / error events fired and so on), but it prevents the crash on release (keeping the same behavior as before).

• String changes made/needed: none

Security Approval Request

• How easily could an exploit be constructed based on the patch?: not easily, finding ways to trigger this is hard and depends on the network.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: all, but issue is more visible in 79+ because of shared stylesheet caching changes
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It should apply cleanly to 79. For 78 I probably need a separate patch, which shouldn't be too hard to create.
• How likely is this patch to cause regressions; how much testing does it need?: not likely, it should only change behavior in the error / crashing condition.

Comment 8

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9163770 [details]
Bug 1649728 - Use WeakPtr in SharedStyleSheetCache::mLoadingDatas. r=heycam,#style

Approved to land

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/10064e664a57f5a297dd25ae8c6ba07c03398b2a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9163770 [details]
Bug 1649728 - Use WeakPtr in SharedStyleSheetCache::mLoadingDatas. r=heycam,#style

Hopefully wallpapers a new topcrash in 79. Approved for 79.0b9.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b24e00d24937

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/10064e664a57

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.