715837 - Crash @ mozilla::net::HttpChannelParent::RecvMarkOfflineCacheEntryAsForeign

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Scoobidiver (away)]

It's #5 top crasher in Fennec 9.0.

Signature 	mozilla::net::HttpChannelParent::RecvMarkOfflineCacheEntryAsForeign More Reports Search
UUID	5c4b82ed-1f85-4823-b897-a07b52120104
Date Processed	2012-01-04 15:38:12.278705
Uptime	183
Last Crash	more than 3 months before submission
Install Age	1.2 weeks since version was first installed.
Install Time	2011-12-27 19:49:06
Product	Fennec
Version	9.0
Build ID	20111216135618
Release Channel	release
OS	Linux
OS Version	0.0.0 Linux 2.6.32.9-g34b306d #1 PREEMPT Sat Aug 20 10:40:49 IST 2011 armv7l
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0x0
App Notes 	
motorola DROIDX
verizon/shadow_vzw/cdma_shadow:2.3.3/4.5.1_57_DX5-35/110820:user/release-keys

Frame 	Module 	Signature [Expand] 	Source
0 	libxul.so 	mozilla::net::HttpChannelParent::RecvMarkOfflineCacheEntryAsForeign 	netwerk/protocol/http/HttpChannelParent.cpp:365
1 	libxul.so 	mozilla::net::PHttpChannelParent::OnMessageReceived 	obj-firefox/ipc/ipdl/PHttpChannelParent.cpp:659
2 	libxul.so 	mozilla::dom::PContentParent::OnMessageReceived 	obj-firefox/ipc/ipdl/PContentParent.cpp:752
3 	libxul.so 	mozilla::ipc::AsyncChannel::OnDispatchMessage 	ipc/glue/AsyncChannel.cpp:294
4 	libxul.so 	mozilla::ipc::RPCChannel::OnMaybeDequeueOne 	ipc/glue/RPCChannel.cpp:433
5 	libxul.so 	RunnableMethod<mozilla::ipc::RPCChannel, bool , Tuple0>::Run 	ipc/chromium/src/base/tuple.h:383
6 	libxul.so 	mozilla::ipc::RPCChannel::DequeueTask::Run 	RPCChannel.h:464
7 	libxul.so 	MessageLoop::RunTask 	ipc/chromium/src/base/message_loop.cc:318
8 	libxul.so 	MessageLoop::DeferOrRunPendingTask 	ipc/chromium/src/base/message_loop.cc:326
9 	libxul.so 	MessageLoop::DoWork 	ipc/chromium/src/base/message_loop.cc:426
10 	libxul.so 	mozilla::ipc::DoWorkRunnable::Run 	ipc/glue/MessagePump.cpp:70
11 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:631
12 	libxul.so 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:245
13 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
14 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:208
15 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:201
16 	libxul.so 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
17 	libxul.so 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:228
...

Comment 1

[Honza Bambas (:mayhemer)]

This can happen only for a top-level document loading channels being satisfied from offline cache.  mChannel had already been nullified from the parent channel.

Marking a cache entry as foreign is triggered from content sink (early) during parsing of the document content on the content process.  It prevents load of the document from the offline cache on next load.  The document is reloaded after that marking.

Problem is that we will simply loop when the cache entry is not marked as foreign because on the next load attempt the conditions leading to mark/reload action will pass again.

We cannot hold the cache entry or the channel on the chrome process to not hold on loads of the same page in other tabs.  We artificially throw the reference to the channel and the cache entry away after the document had been delivered to the content process.

We could delay this artificial release until/after "SetMark" happened to be called when loading a top level doc from offline cache.

Comment 2

[Scoobidiver (away)]

It's still #5 top browser crasher in XUL Fennec 10.0.3esr (340,000 ADU).
The Aurora (800 ADU) and Nightly (500 ADU) population seems unaffected in Native Fennec.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1

- simple patch
- leaves the cleanup logic intact
- to mark an entry offline HttpChannelParent is using a class, provided by nsHttpChannel, having a method MarkForeign()
- the class just keeps what's necessary to perform the op: app cache instance and the key

Comment 4

[Jason Duell]

Comment on attachment 620769 [details] [diff] [review]
v1

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

Looks good.

> The Aurora (800 ADU) and Nightly (500 ADU) population seems unaffected in Native Fennec.

Yes, because they don't use e10s.  We'll need this for B2G, but don't need to backport anywhere.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +160,5 @@
>          mRequestHead.SetHeader(nsHttp::Referer, spec);
>          return NS_OK;
>      }
>  
> +    class OfflineCacheEntryAsForeignMarker {

Add comment:

// This allows cache entry to be marked as foreign even after channel itself
// is gone.  Needed for e10s (see HttpChannelParent::RecvDocumentChannelCleanup)

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 620769 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4cf9833464e

Comment 6

[Geoff Lankow (:darktrojan)]

Backed out, it's burning all the platforms.

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Geoff Lankow (:darktrojan) from comment #6)
> Backed out, it's burning all the platforms.

Except Windows I assume, since it is too much benevolent...  Thanks for the backout.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1.1 (as landed)

https://hg.mozilla.org/integration/mozilla-inbound/rev/f63a2c79ad1b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Original landing and backout:
http://hg.mozilla.org/mozilla-central/rev/b4cf9833464e
http://hg.mozilla.org/mozilla-central/rev/dc4c354a204f

Also, should this have a test?

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f63a2c79ad1b

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen from comment #9)
> Also, should this have a test?

There is a test.  Just don't run on e10s (fennec).  I'm able to make fennec crash w/o this fix quit easily.