927878 - crash in nsDOMOfflineResourceList::ApplicationCacheAvailable(nsIApplicationCache*) when installing an app

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Chris Lord [:cwiiis]]

This bug was filed from the Socorro interface and is 
report bp-26e7edb4-9178-401e-8560-c1bca2131017.
=============================================================


I'm getting this crash when I install an app I've written - it has a valid app cache and works otherwise. Let me know if the crash report is inadequate to investigate this bug. I can't publicly share the url right now.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Can you attach a test case?

Comment 2

[Chris Lord [:cwiiis]]

I will when I find time (soon) - one thing I can say, having the webapp use a different appcache path fixed the issue. So it seems if a page uses appcache and a webapp uses the same appcache, there's some kind of conflict?

Comment 3

[Chris Lord [:cwiiis]]

Sorry, didn't mean to change component... Bugzilla caching at its finest.

Comment 4

[Chris Lord [:cwiiis]]

Go to the attached URL and click 'install' to crash Firefox. Will attach an archive of the files (which have to be hosted on the root of an http or https server).

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Demonstration of crashing bug

Comment 6

[Marco Castelluccio [:marco]]

It's probably bug 910422.
Can you try to:
1) Remove the appcache data from Firefox
2) Reinstall the app without letting Firefox download the appcache data

Comment 7

[Chris Lord [:cwiiis]]

(In reply to Marco Castelluccio [:marco] from comment #6)
> It's probably bug 910422.
> Can you try to:
> 1) Remove the appcache data from Firefox
> 2) Reinstall the app without letting Firefox download the appcache data

That bug title indeed does show the same behaviour. It's worth noting that you don't need to install an app, if you just go to a different page with the same manifest, this will cause the same crash.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Honza, this is a null pointer crash. Could you take a look at this?
I don't see anything guaranteeing that AssociateDocuments(mPreviousApplicationCache);
passes non-null value to the method. We have null checks for mPreviousApplicationCache elsewhere, but
for some reason not there.

Comment 9

[Honza Bambas (:mayhemer)]

The best solution here (I've checked MXR) is to bypass nsOfflineCacheUpdate::AssociateDocuments when the cache is null.  ApplicationCacheAvailable should not be called when there is no cache available.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1

- bypass calling observers when there is no cache to associate with

Comment 11

[Honza Bambas (:mayhemer)]

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

https://tbpl.mozilla.org/?tree=Try&rev=4c6f4e23642c

Comment 12

[Chris Lord [:cwiiis]]

Once this land on central and is verified, can we uplift this to beta and aurora? I think this is a major hindrance to app development (and crashing bugs aren't great regardless)

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Chris Lord [:cwiiis] from comment #12)
> Once this land on central and is verified, can we uplift this to beta and
> aurora? I think this is a major hindrance to app development (and crashing
> bugs aren't great regardless)

Planning on it.  I'll land and request a? tomorrow.

Comment 14

[Honza Bambas (:mayhemer)]

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

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Honza Bambas (:mayhemer)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 892488

User impact if declined: null-deref-crash in some application cache download scenarios

Testing completed (on m-c, etc.): just landed on m-c

Risk to taking this patch (and alternatives if risky): very small, we only bypass a callback that does carry a null ptr (when it should not); according mxr, consumers are OK when this callback is bypassed for null values ; if considered not safe, we can have a branch patch that will just null check in the place of the actual crash. that would fix this bug (in a hacky way) as well and be safer according potential extension compatibility

String or IDL/UUID changes made by this patch: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9ac9f5a95f24
https://hg.mozilla.org/releases/mozilla-beta/rev/5a0e7c8843a2

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5a0e7c8843a2

Comment 19

[Paul Silaghi, QA [:pauly]]

Reproduced on nightly 2013-10-27 when installing the app from the test URL.
Verified fixed FF 28.0a1 (2013-11-14) Win 7 x64

Comment 20

[Bogdan Maris, Desktop QA]

Verified as fixed on Mac OS X 10.8.5, Ubuntu 13.04 x64 and Windows 8 x64 using Firefox 26 beta 6 build.

Comment 21

[Kevin Brosnan [Ex-Mozilla]]

This looks to be verified.

Comment 23

[Manuela Muntean [Away]]

Verified as fixed on Mac OS X 10.7.5, Ubuntu 13.10 x64 and Windows 7 x64 using Firefox 27 beta 6 build.