1576374 - Crash in [@ OOM | large | NS_ABORT_OOM | mozilla::safebrowsing::ProtocolParserProtobuf::AppendStream]

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[[:philipp]]

This bug is for crash report bp-fe0b469b-28f0-4d81-8370-de6e30190824.

Top 10 frames of crashing thread:

0 xul.dll NS_ABORT_OOM xpcom/base/nsDebugImpl.cpp:604
1 xul.dll nsresult mozilla::safebrowsing::ProtocolParserProtobuf::AppendStream toolkit/components/url-classifier/ProtocolParser.cpp:726
2 xul.dll nsresult UrlClassifierDBServiceWorkerProxy::UpdateStreamRunnable::Run toolkit/components/url-classifier/nsUrlClassifierProxies.cpp:93
3 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1175
4 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
5 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:333
6 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308
7 xul.dll static void nsThread::ThreadFunc xpcom/threads/nsThread.cpp:454
8 nss3.dll static void _PR_NativeRunThread nsprpub/pr/src/threads/combined/pruthr.c:397
9 nss3.dll unsigned int pr_root nsprpub/pr/src/md/windows/w95thred.c:137

this out of memory crash is hitting firefox builds on 32bit and 64bit platforms.
in 85% of cases it appears to be a startup crash (happening within the first 60s of launching the browser).

Comment 1

[Dimi Lee [:dimi]]

This bug only happens in windows platform according to the crash reports, so I believe this is not because of server sending too many data (If it was the case, we would see this happening in all the platforms).
I don't think we can really fix the OOM here because the logic in where the crash happens is quite straightforward, so I'll just add fallible to AppendStream.

Comment 2

[Dimi Lee [:dimi]]

Attachment: Bug 1576374 - Use fallible append in ProtocolParser::AppendStream. r?gcp

Comment 3

[Gian-Carlo Pascutto [:gcp]]

https://crash-stats.mozilla.org/report/index/fe0b469b-28f0-4d81-8370-de6e30190824

Crashing in a 64-bit build with 1.3GB "free" RAM is weird, but it does show the page file almost out of space, so it's possible because I have seen Windows do strange things when out of swap space.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42de2e88ff55
Use fallible append in ProtocolParser::AppendStream. r=gcp

Comment 5

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

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/42de2e88ff55

Comment 7

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

Want to request beta uplift? I don't think we can tell if this fixes anything in nightly and the volume is high enough in beta that I'd take the patch if you think it's not too risky.

Comment 8

[Dimi Lee [:dimi]]

(In reply to Liz Henry (:lizzard) from comment #7)

Want to request beta uplift? I don't think we can tell if this fixes anything in nightly and the volume is high enough in beta that I'd take the patch if you think it's not too risky.

Yes, we can uplift this patch. I don't think this is a risky.

Comment 9

[Dimi Lee [:dimi]]

(In reply to Dimi Lee [:dimi][:dlee] from comment #8)

(In reply to Liz Henry (:lizzard) from comment #7)

Want to request beta uplift? I don't think we can tell if this fixes anything in nightly and the volume is high enough in beta that I'd take the patch if you think it's not too risky.

Yes, we can uplift this patch. I don't think this is a risky.

But this patch doesn't really fix the OOM issue, this just makes sure Safe Browsing is aware of OOM and returns an error when it happens.
Do you think this is worth uplifting?

Comment 10

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

Ah, in that case let's let it stay in 71. Thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for ESR68 uplift? It grafts cleanly as-landed.

Comment 12

[Dimi Lee [:dimi]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

Is this something we should consider for ESR68 uplift? It grafts cleanly as-landed.

As mentioned in Comment 9, this patch is more like an error handling, not really a fix for the OOM problem.
Since this patch is not risky, if you think we should uplift this one, I'll do it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I think not crashing is still a better outcome here, unless you're suggesting that we're just pushing the OOM crash somewhere else instead?

Comment 14

[Dimi Lee [:dimi]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

I think not crashing is still a better outcome here, unless you're suggesting that we're just pushing the OOM crash somewhere else instead?

If the system keeps running out of memory, we might still end up crashing somewhere else. It is also possible that the OOM only occurs for a short period of time and this patch can help not crash Firefox when a Safe Browsing update happen during that low memory period. I can't tell what is the case from the crash reports

After thinking for a while, I think this patch is still worth uplifting because the risk of the patch is very low and it "MAY" help fix crashes in certain scenarios.

Comment 15

[Dimi Lee [:dimi]]

Comment on attachment 9099233 [details]
Bug 1576374 - Use fallible append in ProtocolParser::AppendStream. r?gcp

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: If a system is OOM, and meantime a SafeBrowsing update occurs, Firefox may crash
• User impact if declined: Firefox crashes when the system is OOM
• Fix Landed on Version: 71
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch only changes how we deal with the case when ProtocolParser::AppendStream API returns OOM error
• String or UUID changes made by this patch: None

Comment 16

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

Comment on attachment 9099233 [details]
Bug 1576374 - Use fallible append in ProtocolParser::AppendStream. r?gcp

Crash fix, let's uplift for ESR 68.3.

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/3c890dc2a3bf

Comment 18

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Hello,

Would it be possible to offer QA some STR in order to verify this issue?

Thank you in advance!

Comment 19

[[:philipp]]

sorry, i only filed the bug based on crash statistics. i have no STR available and most user comments for those crashes aren't that helpful in figuring out how to trigger this - comment 15 described a scenario where this crash might occur though.

we can probably verify this bug as well according to crash stats, as there are no new crashes recorded during the 71.0b cycle (there were plenty on beta before).