Closed Bug 1508740 Opened Last year Closed 6 months ago

Crash in OOM | large | NS_ABORT_OOM | nsTSubstring<T>::SetLength

Categories

(Core :: Storage: localStorage & sessionStorage, defect, critical)

64 Branch
Unspecified
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: lizzard, Assigned: janv)

References

Details

(Keywords: crash, regression, topcrash)

Crash Data

This bug was filed from the Socorro interface and is
report bp-8ce355b6-ea89-4869-b299-0850a0181120.
=============================================================


This is a top crash for Fennec on release. 
Not sure if it should go in Firefox for Android component or in Networking::Websockets. Filed as a followup from bug 1498194.  


Top 10 frames of crashing thread:

0 libxul.so NS_ABORT_OOM xpcom/base/nsDebugImpl.cpp:628
1 libxul.so nsTSubstring<char16_t>::SetLength xpcom/string/nsTSubstring.cpp:847
2 libxul.so IPC::ParamTraits<nsTSubstring<char16_t> >::Read ipc/glue/IPCMessageUtils.h:462
3 libxul.so mozilla::net::WebSocketFrameRunnable::~WebSocketFrameRunnable netwerk/protocol/websocket/WebSocketEventService.cpp:70
4 libxul.so mozilla::dom::PBackgroundStorageParent::OnMessageReceived ipc/ipdl/PBackgroundStorageParent.cpp:461
5 libmozglue.so RedBlackTree<arena_chunk_map_t, ArenaAvailTreeTrait>::MoveRedRight 
6 libmozglue.so RedBlackTree<arena_chunk_map_t, ArenaAvailTreeTrait>::MoveRedLeft 
7 libc.so libc.so@0x4943a 
8 libxul.so mbrtoc32 
9 libxul.so mbrtoc32 

=============================================================
WebSocketFrameRunnable is created only in WebSocketEventService::FrameReceived() and WebSocketEventService::FrameSent() if there is any nsIWebSocketEventListener registered at the service. I couldn't find any code using this service. Honza, am I right that devtools doesn't use nsIWebSocketEventService at all? If so, it seems that the call stack isn't correct.
Flags: needinfo?(odvarko)
I am not entirely sure. DevTools are using WebSockets e.g. for debugger transport layer.

@Alex, I am thinking about this code:
https://searchfox.org/mozilla-central/source/devtools/shared/security/socket.js#682

Are there any WebSockets listeners involved? Or any other parts of DevTools could be involved?

Honza
Flags: needinfo?(odvarko) → needinfo?(poirot.alex)
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #2)
> I am not entirely sure. DevTools are using WebSockets e.g. for debugger
> transport layer.
> 
> @Alex, I am thinking about this code:
> https://searchfox.org/mozilla-central/source/devtools/shared/security/socket.
> js#682

Sorry about the delay!!

WebSockets shouldn't be used anywhere for DevTools protocol.
The DevTools server do support WebSocket, but that is an optional feature, that I believe is only used optionally by contributors using launchpad to run our tools.
See this doc:
  https://firefox-dev.tools/debugger.html/docs/getting-setup.html#firefox-using-websocket-transport

On android, we are using a unix domain socket to communicate through ADB:
  https://searchfox.org/mozilla-central/source/mobile/android/app/mobile.js#369-371
  https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/RemoteDebugger.js#273-283
Same thing for geckoview:
  https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewRemoteDebugger.jsm#72-85

Now, some people for some unexpected reason may try to enable websocket devtools server on android (via custom preferences), but that would be only a couple of people doing unexpected/undocumented things!

Otherwise, we are not using nsIWebSocketEventService anywhere in DevTools.
We are using WebSocket.createServerWebSocket:
  https://searchfox.org/mozilla-central/source/dom/webidl/WebSocket.webidl#71-77
  https://searchfox.org/mozilla-central/source/devtools/server/socket/websocket-server.js#208
But again, that's related to the optional feature I just described, which isn't used by default and it seems very unlikely that anyone is using it on Android.
Flags: needinfo?(poirot.alex)
P1 given the high crash rate on fennec
Assignee: nobody → michal.novotny
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [necko-triaged]
Alexandre, my original question was about using WebSocketEventService not WebSocket server/client. WebSocketEventService notifies registered listeners about all sent/received messages by WebSocket connections associated with a given window. It was created for devtools so the developers can see all WebSocket traffic in the network panel. But it looks like this functionality was never implemented in network panel and I couldn't use any other code using WebSocketEventService.
Flags: needinfo?(poirot.alex)
It was used by an old privileged add-on:
  https://github.com/firebug/websocket-monitor/blob/014afce40ea280eafae2cbd8d676ab9ec914bc8c/lib/wsm-actor.js#L61-L80
But as we deprecated all these add-ons in favor of WebExtension, this is no longer used.
We never backported this add-on into DevTools, nor exposed this API through WebExtension.
Honza may better know about eventual plan to implement this feature into Firefox again.
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #6)
> But as we deprecated all these add-ons in favor of WebExtension, this is no
> longer used.
> We never backported this add-on into DevTools, nor exposed this API through
> WebExtension.
> Honza may better know about eventual plan to implement this feature into
> Firefox again.
Yes, this is correct. The WebSocketMonitor extension isn't ported into the world of WebExtensions yet, but it's been requited by users many times and we definitely plan to do it at some point. It'll be based on the same service + new WebExtension API that expose it.

Honza
Honza Bambas pointed out that frame 3 is invalid (probably shared code due to optimization) and important is frame 4. The message originates at https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/dom/storage/StorageIPC.cpp#324. Jan, I think you're familiar with that code. Can you have a look?
Assignee: michal.novotny → nobody
Status: ASSIGNED → NEW
Component: Networking: WebSockets → DOM
Flags: needinfo?(jvarga)
Whiteboard: [necko-triaged]
This problem has been described in bug 1475218 comment 9.
New localStorage implementation should land following days, however preffed-off for now. We plan to enable it in next cycle (FF 66). We'll see if it helps to lower down number of crashes related to this problem.
Flags: needinfo?(jvarga)
Per comment 9, looks like DOM: Web Storage may be a better fit.
Component: DOM → DOM: Web Storage
Priority: P1 → --
Depends on: 1517090
Priority: -- → P2

Note to myself, compression may help here.

This is currently #6 overall on Fennec 66.0.2 release.

Jan, the crash rate is indeed lower than before, but since it is still crashing, do you have ideas on additional work that could be done here for 67 beta?

Flags: needinfo?(jvarga)

Yeah, it seems the new architecture helped a bit.
The next thing that could help is compression of data. Data would be compressed before it's sent over IPC.
That's bug 1513937.

I plan to work on that in near future to mitigate bug 1534222 and improve performance, reduce memory footprint and store less data on disk in general.

Flags: needinfo?(jvarga)

There's nothing we can do in the 67 timeframe. The new localStorage implementation intends to ship in 68 which will help here (see comment 14 for notes about further work).

Depends on: 1513937
Depends on: 1546310
Depends on: 1546723
Duplicate of this bug: 1552340
Crash Signature: [@ OOM | large | NS_ABORT_OOM | nsTSubstring<T>::SetLength] → [@ OOM | large | NS_ABORT_OOM | nsTSubstring<T>::SetLength] [@ OOM | large | NS_ABORT_OOM | mozilla::dom::LSValue::LSValue]
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Crash Signature: [@ OOM | large | NS_ABORT_OOM | nsTSubstring<T>::SetLength] [@ OOM | large | NS_ABORT_OOM | mozilla::dom::LSValue::LSValue] → [@ OOM | large | NS_ABORT_OOM | nsTSubstring<T>::SetLength] [@ OOM | large | NS_ABORT_OOM | mozilla::dom::LSValue::LSValue]

This is now fixed on Nightly.

Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

What bug fixed this?

Flags: needinfo?(jvarga)

Mainly bug 1546723.

Flags: needinfo?(jvarga)

Yeah, there are still some crashes, but they happen much less often. One of them can be mitigated by avoiding compression if we can't allocate a memory buffer for that. I'm working on a patch for that.

Flags: needinfo?(jvarga)

Bug 1546723 is also in 68 so calling that fixed too.

Priority: P2 → --
You need to log in before you can comment on or make changes to this bug.