760955 - crash in mozilla::net::nsHttpChannel::InstallOfflineCacheListener

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Scoobidiver (away)]

It first appeared in FennecAndroid 15.0a1/20120601. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3aa566994890&tochange=73783bf75c4c
It's likely a regression from bug 722034 or bug 746018.

Signature 	mozilla::net::nsHttpChannel::InstallOfflineCacheListener More Reports Search
UUID	9d6c5a4b-292e-40f3-855d-9d6c22120602
Date Processed	2012-06-02 20:10:32
Uptime	202
Last Crash	4.3 minutes before submission
Install Age	5.1 hours since version was first installed.
Install Time	2012-06-02 15:03:45
Product	FennecAndroid
Version	15.0a1
Build ID	20120602030527
Release Channel	nightly
OS	Linux
OS Version	0.0.0 Linux 2.6.35.7-I9100UHKI3-CL565937 #2 SMP PREEMPT Thu Sep 8 21:17:57 KST 2011 armv7l
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0x0
App Notes 	
AdapterVendorID: smdkc210, AdapterDeviceID: GT-I9100.
AdapterDescription: 'Model: 'GT-I9100', Product: 'GT-I9100', Manufacturer: 'samsung', Hardware: 'smdkc210''.
samsung GT-I9100
samsung/GT-I9100/GT-I9100:2.3.4/GINGERBREAD/UHKI3:user/release-keys
EMCheckCompatibility	True

Frame 	Module 	Signature 	Source
0 	libxul.so 	mozilla::net::nsHttpChannel::InstallOfflineCacheListener 	netwerk/protocol/http/nsHttpChannel.cpp:3843
1 	libxul.so 	mozilla::net::nsHttpChannel::CallOnStartRequest 	netwerk/protocol/http/nsHttpChannel.cpp:975
2 	libxul.so 	mozilla::net::nsHttpChannel::ContinueProcessNormal 	netwerk/protocol/http/nsHttpChannel.cpp:1440
3 	libxul.so 	mozilla::net::nsHttpChannel::ProcessNormal 	netwerk/protocol/http/nsHttpChannel.cpp:1375
4 	libxul.so 	mozilla::net::nsHttpChannel::ProcessResponse 	netwerk/protocol/http/nsHttpChannel.cpp:1289
5 	libxul.so 	mozilla::net::nsHttpChannel::OnStartRequest 	netwerk/protocol/http/nsHttpChannel.cpp:4747
6 	libxul.so 	nsInputStreamPump::OnStateStart 	netwerk/base/src/nsInputStreamPump.cpp:416
7 	libxul.so 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:367
8 	libxul.so 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:81
9 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:624
10 	libxul.so 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:213
11 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:82
12 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:208
13 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:201
14 	libxul.so 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:163
15 	libxul.so 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:256
16 	libxul.so 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3786
17 	libxul.so 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3863
18 	libxul.so 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3939
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Anet%3A%3AnsHttpChannel%3A%3AInstallOfflineCacheListener

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Here's the offending changeset:
http://hg.mozilla.org/mozilla-central/rev/0f3f9ff4439d#l1.10

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Fix null pointer deref in InstallOfflineCacheEntry

Comment 3

[Scoobidiver (away)]

It's #1 top crasher in FennecAndroid 16.0a1.

Comment 4

[Aaron Train [:aaronmt]]

STR: 
i. Install Aurora or Nightly on Android
ii. https://touch.betfair.com/, allow for offline data storage via prompt

Comment 6

[u408661]

Comment on attachment 629677 [details] [diff] [review]
Fix null pointer deref in InstallOfflineCacheEntry

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

I presume (with the exception of the dump shown below) that you intended to keep the do_print statements in, to make reading the test output easier. If not, please remove them. r=me with the changes below.

::: netwerk/test/unit/test_cacheForOfflineUse_no-store.js
@@ +1,1 @@
> +u"se strict"; 

This is wrong. Perhaps it's just an artifact of how you created the patch, but please make sure this isn't committed with the double quotes.

@@ +43,5 @@
> +function asyncCheckCacheEntryExistance(entryName, shouldExist)
> +{
> +  var listener = new CacheListener();
> +  listener.onCacheEntryAvailable = function(descriptor, accessGranted, status) {
> +    dump("foo");

Get rid of this

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 629677 [details] [diff] [review]
Fix null pointer deref in InstallOfflineCacheEntry

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

::: netwerk/test/unit/test_cacheForOfflineUse_no-store.js
@@ +1,1 @@
> +u"se strict"; 

Nick, thank you for pointing out the typo. It should be "use strict"; WITH the quotes, and in particular with the letter u inside the quotes.

@@ +43,5 @@
> +function asyncCheckCacheEntryExistance(entryName, shouldExist)
> +{
> +  var listener = new CacheListener();
> +  listener.onCacheEntryAvailable = function(descriptor, accessGranted, status) {
> +    dump("foo");

Why do you have foo?

Comment 9

[Marcia Knous [:marcia]]

On Firefox side:

mozilla::net::nsHttpChannel::InstallOfflineCacheListener() - 22 trunk/16 Aurora crashes in the last week.

mozilla::net::nsHttpChannel::InstallOfflineCacheListener  - 10 trunk

mozilla::net::nsHttpChannel::InstallOfflineCacheListener has more volume on FennecAndroid side so perhaps it doesn't have to be tracked on the desktop side for FF 15.

Comment 10

[Jason Smith [:jsmith]]

k9o nomination - this causing crashes in web apps. App cache is likely to be used in web apps, so this is a likely use case.

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

k9o- This issue is a quality issue that is already tracked for Fx15.

Comment 14

[JP Rosevear [:jpr]]

Any reason not to land the fix and get an uplift to Aurora?

Comment 15

[Justin Lebar (not reading bugmail)]

If this is indeed the same as bug 769211, it's blocking an important b2g demo for tomorrow.  I'm going to verify that this fixes bug 769211; if so, we'll need this landed very soon (perhaps directly on m-c, if that's possible).

Comment 16

[Justin Lebar (not reading bugmail)]

I verified that this fixes bug 769211.  I'd presume it also fixes bug 769210; I don't have a device or build handy to test on.

So there's less pressure to check this in, I've pushed the code fix to the B2G repository.  I didn't check in the tests because I didn't want to conflict with your review changes.  Anyway, we don't run xpcshell tests on b2g.

https://github.com/mozilla-b2g/mozilla-central/commit/4085977dd826686ca4ab6ffe023f61488773e1b9

Comment 17

[Aaron Train [:aaronmt]]

This is breaking mobile Twitter if you opt-in to storing offline data.

Comment 18

[JP Rosevear [:jpr]]

(In reply to Justin Lebar [:jlebar] from comment #16)
> So there's less pressure to check this in, I've pushed the code fix to the
> B2G repository.  

For B2G, but this is busting fennec native as well, right now.

Comment 19

[Jason Smith [:jsmith]]

(In reply to JP Rosevear [:jpr] from comment #18)
> (In reply to Justin Lebar [:jlebar] from comment #16)
> > So there's less pressure to check this in, I've pushed the code fix to the
> > B2G repository.  
> 
> For B2G, but this is busting fennec native as well, right now.

And busting the destkop web runtime.

Comment 20

[Justin Lebar (not reading bugmail)]

Andreas just asked me to land this patch on m-c.  I'm about to leave for dinner, but I'll do it when I get back if Brian doesn't get to it first.

Comment 21

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/mozilla-central/rev/90f4210aa8d6

Comment 22

[Tony Chung [:tchung]]

Verified fix against 6-29-2012 b2g build, otoro device.  Twitter does not crash anymore. 

commit 4085977dd826686ca4ab6ffe023f61488773e1b9
Author: Brian Smith <[e-mail]>
Date:   Thu Jun 28 08:45:07 2012 -0400

    Bug 760955: Fix null pointer deref in InstallOfflineCacheEntry, r=hurley
    
    Checked in directly to mozilla-b2g's git repository, due to bug 769211.  a=cjones over IRC.

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Brian or Justin - Can you request Aurora approval with the risk assessment? This crash is happening in Fennec 15 and we'd like to get it uplifted soon.

Comment 24

[Justin Lebar (not reading bugmail)]

Brian made it clear to me that I have no idea what's going on here, so this is all on him.

Comment 25

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I have asked for the change that caused the regression to be backed out of aurora in bug 722034.

Comment 26

[Chris Peterson [:cpeterson]]

This crash is still the #1 topcrash on Fennec 15. The Aurora 15 backout landed just a few minutes ago, as per Bug 722034 Comment 79.

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 629677 [details] [diff] [review]
Fix null pointer deref in InstallOfflineCacheEntry

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 722034
User impact if declined: Crashes
Testing completed (on m-c, etc.): This patch has been on mozilla-central and aurora for a while now.
Risk to taking this patch (and alternatives if risky): This is a very, very safe patch.
String or UUID changes made by this patch: none.

Comment 28

[Scoobidiver (away)]

The patch is already in 16 Beta and Aurora 17 (see the target milestone).

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 629677 [details] [diff] [review]
Fix null pointer deref in InstallOfflineCacheEntry

Yup, this landed on central when 16 was still there, so no need to uplift.