502403 - Crash when selecting advanced pane in preferences dialog and having Google Gears v0.5.25.0 installed [@ nsOfflineCacheDevice::GetInstance]

Status: RESOLVED INVALID

(Firefox :: Extension Compatibility, defect)

Description

[NHLfan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.9.1) Gecko/20090624 Firefox/3.5 GTB5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.9.1) Gecko/20090624 Firefox/3.5 GTB5

If I want to open the options menu (tools -> options) then firefox 3.5 keeps crashing.
It doesn't matter on which page I currently am, it happens on every webpage

Reproducible: Always

Steps to Reproduce:
1. click on tools
2. click on options
3. crashed / crash report window opens
Actual Results:  
firefox disappeared and instead the crashreporter appeared

Expected Results:  
it should have opened the options window

Comment 1

[NHLfan]

Ok, I tested a little further and checked the extensions.
I have installed a (unofficially) developer version of google gears (see http://groups.google.com/group/gears-users/msg/14d691d48e647f33) with the version: 0.5.25.0;developer;opt;win32;firefox

I deactivated this extension, restarted firefox and tried again to access the options window and now it works

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Reporter, can you please give us a crash id which you can find when typing about:crashes into the location bar? The latest one would be sufficient. Thanks!

Lets reopen the bug due to an extension should not crash Firefox. Let us investigate this problem.

Comment 3

[NHLfan]

Hi,
Here is the crash ID

371ea2d2-fbea-46f2-91b2-166b72090705

thanks!

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'll check tomorrow if I'm able to reproduce the crash.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #3)
> 371ea2d2-fbea-46f2-91b2-166b72090705

bp-371ea2d2-fbea-46f2-91b2-166b72090705

0  	xul.dll  	ns_if_addref<nsOfflineCacheDevice*>  	 obj-firefox/dist/include/xpcom/nsISupportsUtils.h:114
1 	xul.dll 	nsOfflineCacheDevice::GetInstance 	netwerk/cache/src/nsDiskCacheDeviceSQL.cpp:954
2 	xul.dll 	nsOfflineCacheDeviceConstructor 	netwerk/build/nsNetModule.cpp:205
3 	xul.dll 	nsGenericFactory::CreateInstance 	obj-firefox/xpcom/build/nsGenericFactory.cpp:80
4 	xul.dll 	nsComponentManagerImpl::CreateInstance 	xpcom/components/nsComponentManager.cpp:1601
5 	xul.dll 	xul.dll@0x972aa7

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, this happens when you wanna show the advanced pane inside the preferences dialog.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'll attach a full log from windbg in a minute. Josh, it would be nice if you could have a look at it.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Windbg debug log

Comment 9

[timeless]

APPLICATION_FAULT_INVALID_POINTER_READ_Ed20.dll!Unloaded

makes me wonder what exactly ed20.dll is...

can you run firefox from windbg and provide that log?

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

There is no Ed20.dll on my system. Also it is not loaded on startup. The only DLL I can find is RichEd20.dll. No idea if Windbg is confused here?

Comment 11

[timeless]

windows doesn't try very hard to retain lists of unloaded modules...
can you just run firefox from windbg w/ logging, crash it and attach the log?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #11)
> windows doesn't try very hard to retain lists of unloaded modules...
> can you just run firefox from windbg w/ logging, crash it and attach the log?

Is that different from attaching to the process? If not everything should be in my attached windbg log.

Comment 13

[timeless]

it is, hence my request :)

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Windbg v2

Josh, something was definitely wrong with the modules yesterday. Some minutes ago I run the same steps in my fresh build Shiretoko debug build on XP and there is no such dll listed anymore. Instead we really crash in necko.dll.

Comment 15

[timeless]

so... the cacheservice claims to be threadsafe. I claim it isn't really accessing mOfflineDevice in a threadsafe manner. CreateOfflineDevice can be called from all over.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So this should probably happen on all platforms, right? But since there is no development version of Google Gears for OS X I'm not able to test it.

Comment 17

[spAmM3n0t]

(In reply to comment #16)
> So this should probably happen on all platforms, right? But since there is no
> development version of Google Gears for OS X I'm not able to test it.

It happens on OS X as well, both with 0.5.25.0 and 0.5.26.0.
You can obtain a build from here (it's even a debug build iirc): http://groups.google.com/group/gears-users/browse_thread/thread/1c11eb7bca12ee3e/bacb5088b9a6c86f

Comment 18

[Boris Zbarsky [:bzbarsky]]

This sounds really bad.  Honza, can you please look into this?

Comment 19

[Honza Bambas (:mayhemer)]

-> me

Comment 20

[Honza Bambas (:mayhemer)]

This really is bad. google gears implements "@mozilla.org/network/cache-service;1" and overwrites this way the cache service in Firefox (Gecko). In nsOfflineCacheDevice::GetInstance, where we crash, we do static cast to nsCacheService (to our implementation!) on object we get from do_GetService(kCacheServiceCID) to call the CreateOfflineDevice() private method. The object we get is google gears implementation and properties of the object are indeed messed up. We pass the condition if (mOfflineDevice) because there are some non-null bytes; then we do AddRef() on it -> crash.

http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp#947

What's curious is that code on mozilla-central is the same as on mozilla-1.9.1. We should crash the same way... Any idea here? Maybe some restriction in XPCOM not to let extensions register it's own components we already have registered?

The solution is to have a new interface and publish the CreateOfflineDevice() method to it; then do QI instead of static_casts (what's ugly anyway).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Er... We're getting the cache service by _CID_ here, not contractid.  Same CID means same implementation.  If Google Gears is registering their different implementation for the same CID, that's totally a bug in Google Gears, no?

Comment 22

[Honza Bambas (:mayhemer)]

True. But I'd say it's total hack, not bug... That's intentional.

Comment 23

[Boris Zbarsky [:bzbarsky]]

OK, so they do in fact register with our CID.  For revision 3379 in the Google Gears SVN repo, file http://gears.googlecode.com/svn/trunk/gears/localserver/firefox/cache_intercept.cc, we have:

Lines 78-87: define the CID and contract for our cache service
Line 673: get our cache service (by CID, correctly)
Lines 694-697: register their own factory using our CID and contract

They should be using a different CID for that last bit.

We could still try to make the changes Honza proposes, but this really is a bug in Gears, pure and simple.  In particular, Honza's fix would presumably just disable the offline cache when Gears is installed, which is not the right thing either, right?

Honza, "total hack" makes no sense.  The only place where we get the cache service by CID is right here, so if they overrode just the contract as they're supposed to everything but this one callsite would work fine and this callsite wouldn't crash.  So yes, it's a bug.  It's a clear violation of XPCOM rules.  Would you call it a total hack if they were handing back non-addrefed objects from getters?  Or not copying strings on get?  Or returning NS_OK but not filling in out params?  These are all no worse than what they're doing.

Comment 24

[Boris Zbarsky [:bzbarsky]]

kev, do you mind getting in touch with the Gears folks about this?

Comment 25

[Kev Needham [:kev]]

forwarding now :)

Comment 26

[Honza Bambas (:mayhemer)]

(In reply to comment #23)
> Honza, "total hack" makes no sense.

I didn't know there was a way to change the cid bits and problem would be solved with leaving the contract id override working. Taking back :)

Comment 27

[Aaron Boodman]

Forgive my lack of familiarity with this code (either side of it, really), but iirc, what we are trying to do here is wrap the built in cache service. If we were to register by our own class id, I wonder if Gears would still work? I suppose it depends on whether Mozilla usually asks for this service by contract ID or class ID.

Comment 28

[Nochum Sossonko [:Natch]]

Aaron see the last paragraph of comment 23, you should override the contract id, *but not* the class id. It would still work, and then code that makes assumptions about its' own classes would work as well...

Comment 29

[Aaron Boodman]

Ok, I will try this out locally and see if things still appear to work.

Is anyone aware of an HTML5 test page I can use to verify the Firefox feature works as well?

Comment 30

[Aaron Boodman]

It appears to work:

http://aaronboodman.com/z_dropbox/gears-win32-dbg-0.5.27.0.xpi

...if anyone wants to try it.

Comment 31

[Honza Bambas (:mayhemer)]

(In reply to comment #30)
> It appears to work:
> 
> http://aaronboodman.com/z_dropbox/gears-win32-dbg-0.5.27.0.xpi
> 
> ...if anyone wants to try it.

It's forbidden...

Comment 32

[Aaron Boodman]

Balls, fixed now.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Aaron, where can we get the fixed version to run a test against?

Comment 34

[Aaron Boodman]

The fixed version was linked in comment #30:

http://aaronboodman.com/z_dropbox/gears-win32-dbg-0.5.27.0.xpi

The file was originally not marked to be served, so it was returning 403: Forbidden. But I fixed that. You may have to hard-refresh to see the change if you already tried to load it.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Aaron. That looks good. The crash is gone.

Honsa, I believe we can close this bug now?

Comment 36

[Aaron Boodman]

FWIW, the nasty hack was not as tasteless as it at first appeared. It turns out
that we needed it this way in FF2. Obviously FF2 is old and nobody probably
uses it anymore, but I thought you might like to know that it appears the hack
was at least necessary at one point. 

See also:
http://code.google.com/p/gears/source/browse/trunk/gears/localserver/firefox/cache_intercept.cc#694

Comment 37

[NHLfan]

thanks Aaron.
It works for me.

Still I hope that Google will publish a official version soon.

Comment 38

[spAmM3n0t]

Works like a charm again on OS X as well.
If anyone is interested:
http://files.getdropbox.com/u/699312/gears-osx-opt-0.5.29.0.xpi

Comment 39

[Honza Bambas (:mayhemer)]

Fixed by fixing gears.