Closed Bug 1576374 Opened 1 year ago Closed 10 months ago

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

Categories

(Toolkit :: Safe Browsing, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 71+ fixed
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: philipp, Assigned: dimi)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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).

Priority: -- → P1

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.

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.

Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42de2e88ff55
Use fallible append in ProtocolParser::AppendStream. r=gcp

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

Keywords: regression
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

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.

Flags: needinfo?(dlee)

(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.

Flags: needinfo?(dlee)

(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?

Flags: needinfo?(lhenry)

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

Flags: needinfo?(lhenry)

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

Flags: needinfo?(dlee)

(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.

Flags: needinfo?(dlee) → needinfo?(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?

Flags: needinfo?(ryanvm)

(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 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
Attachment #9099233 - Flags: approval-mozilla-esr68?

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

Crash fix, let's uplift for ESR 68.3.

Attachment #9099233 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Hello,

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

Thank you in advance!

Flags: needinfo?(madperson)

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).

Flags: needinfo?(madperson)
You need to log in before you can comment on or make changes to this bug.