User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:18.0) Gecko/18.0 Firefox/18.0 Build ID: 20120918030553 Steps to reproduce: Set the key 'security.mixed_content.block_active_content' to true. Browse to https://panopticlick.eff.org/ and click 'TEST ME'. This was tested in safe mode, and with a completely new profile. Actual results: Browser crashed. See https://crash-stats.mozilla.com/report/index/bp-3c0ba554-7ce2-40c7-a068-5aee62120918 . Expected results: No crash.

Looks like a missing null check for aContentLocation: http://hg.mozilla.org/mozilla-central/annotate/e4757379b99a/content/base/src/nsMixedContentBlocker.cpp#l130 CSP does this check first thing: http://hg.mozilla.org/mozilla-central/annotate/e4757379b99a/content/base/src/nsCSPService.cpp#l63 Should we do the same thing here?

No crash on Windows.

I experienced the crash on Mac OS X 10.7. Last few lines from the command line... WARNING: NS_ENSURE_TRUE(aURI) failed: file /Users/tvyas/dev/mozilla-central/caps/src/nsScriptSecurityManager.cpp, line 2048 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /Users/tvyas/dev/mozilla-central/extensions/cookie/nsPermissionManager.cpp, line 905 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /Users/tvyas/dev/mozilla-central/extensions/permissions/nsContentBlocker.cpp, line 249 WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /Users/tvyas/dev/mozilla-central/extensions/permissions/nsContentBlocker.cpp, line 211 I tried adding if (!aContentLocation) return NS_ERROR_FAILURE; to http://hg.mozilla.org/mozilla-central/annotate/e4757379b99a/content/base/src/nsMixedContentBlocker.cpp#l90 and rebuilt. All the mixed content tests still pass and clicking "test me" on https://panopticlick.eff.org/ no longer causes the browser to crash. The test finishes.

I can crash reliably while loading gmail (and the pref set to true): bp-71194fb1-e057-49f7-af98-1370c2120918

crashes on facebook, too. https://crash-stats.mozilla.com/report/index/bp-4f435c1b-cc05-4c17-95d8-017a12120919 Tested with Mac OS X 10.8 and Windows 7.

I'm working on fixing this.

Backtrace: http://pastebin.mozilla.org/1832008 Adding a null check will fix the issue: + if (!aContentLocation || NS_FAILED(aContentLocation->SchemeIs("https", &isHttps)) || isHttps) { - if(NS_FAILED(aContentLocation->SchemeIs("https", &isHttps)) || isHttps) { to http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsMixedContentBlocker.cpp#130 But looking at the backtrace, I think there might be another issue elsewhere in the code (why is aContentLocation null? Perhaps an issue in nsObjectLoadingContent.cpp). If I don't figure this out by tomorrow, I'll update the bug with the simple patch to aContentLocation so that we can land that in Nightly and people can experiment with the feature without it crashing their browser. And I can continue investigating.

Looks like there has been a change to panoptclick.eff.org, and hence I am longer able to reproduce the crash. I tried facebook and gmail as well. Is there a developer/older version of panopticlick.eff.org that I can use?

Tanvi, were you able to reproduce this when signed in on a Google+ profile page?

I can reproduce using gmail on a Mac. But not gmail on Linux. I went through my backtrace and traced through the code that resulted in the null aContentLocation. It is not obvious to me that aContentLocation (this->mURI) should have been set in any of the functions before it reached nsMixedContentBlocker.cpp. I have attached a patch. I'm still testing it out and will push to try.

(In reply to Tanvi Vyas from comment #11) > Created attachment 663487 [details] [diff] [review] > WIP Patch > > I can reproduce using gmail on a Mac. But not gmail on Linux. I went > through my backtrace and traced through the code that resulted in the null > aContentLocation. It is not obvious to me that aContentLocation > (this->mURI) should have been set in any of the functions before it reached > nsMixedContentBlocker.cpp. > > I have attached a patch. I'm still testing it out and will push to try. Adding johns, since he recently refactored nsObjectLoadingContent.cpp, and perhaps he may know if this->mURI should have been set there.

Another backtrace for gmail.com - http://pastebin.mozilla.org/1834889 The opportunity to set this->mURI is in one of these functions - * CheckProcessPolicy() in nsObjectLoadingContent.cpp:1150 * LoadObject in nsObjectLoadingContent.cpp:1624 * LoadObject in nsObjectLoadingContent.cpp:1490 * StartObjectLoad in nsHTMLSharedObjectElement.cpp:475 * StartObjectLoad nsHTMLSharedObjectElement.cpp:112 The relevant part of the backtrace: #5 0x0000000101b967c3 in nsObjectLoadingContent::CheckProcessPolicy (this=0x1180c1fa8, aContentPolicy=0x7fff5fbe9b4e) at /Users/tvyas/dev/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:1150 #6 0x0000000101b93e36 in nsObjectLoadingContent::LoadObject (this=0x1180c1fa8, aNotify=true, aForceLoad=false, aLoadingChannel=0x0) at /Users/tvyas/dev/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:1624 #7 0x0000000101b93783 in nsObjectLoadingContent::LoadObject (this=0x1180c1fa8, aNotify=true, aForceLoad=false) at /Users/tvyas/dev/mozilla-central/content/base/src/nsObjectLoadingContent.cpp:1490 #8 0x0000000101ed7bfb in nsHTMLSharedObjectElement::StartObjectLoad (this=0x1180c1f30, aNotify=true) at /Users/tvyas/dev/mozilla-central/content/html/content/src/nsHTMLSharedObjectElement.cpp:475 #9 0x0000000101eda8da in nsHTMLSharedObjectElement::StartObjectLoad (this=0x1180c1f30) at /Users/tvyas/dev/mozilla-central/content/html/content/src/nsHTMLSharedObjectElement.cpp:112

if (!aContentLocation || NS_FAILED(aContentLocation->SchemeIs("https", &isHttps)) || isHttps) { return NS_OK; } Should we return NS_OK or return NS_ERROR_FAILURE? If we return NS_OK, would we be allowing a mixed content object load? If we return NS_ERROR_FAILURE, would we be rejecting https object loads? Could this failure be occurring where the plugin is loading and not the actual http/https resource? If so, then a null aContentLocation makes sense. Still investigating.

aContentLocation can legitimately be null for plugins, for instance: > <object type="application/x-java-vm"> > <param name="code" value="applet.class"> > <object> Will load java, but will not open a channel or have a URI associated with it. aRequestingLocation and aRequestContext should still be set if this is in a normal web context.

Also - for URI-less plugins, we will only call shouldProcess() (as we never 'load' a channel), but nsMixedContentBlocker's shouldProcess() is just a |return shouldLoad(...)| at the moment.

Thanks John for your help! I've updated the patch to return OK if we have a plugin with a null aContentLocation. It returns a failure when aContentLocation is not set for any other type. Push to try: https://tbpl.mozilla.org/?tree=Try&rev=449d808b2c5c

Comment on attachment 663602 [details] [diff] [review] Patch This looks fine, though we might want to put this check in shouldProcess instead, since shouldLoad should always have a URI regardless of type. (There's also a few missing brackets for the outer |if|) I'm going to pass this to bz for review however, as I'm not too familiar with the CSP stuff.

Sorry I missed the brackets; not sure why that still worked. I've updated to patch and re-pushed to try (https://tbpl.mozilla.org/?tree=Try&rev=f0cdc9d33a5b ). I can move this code to ShouldProcess() if preferred. r? to Olli, since he did the reviews for bug 62178.

Comment on attachment 664205 [details] [diff] [review] Patch Um, nsIContentPolicy.idl explicitly says that aContentLocation must not be null.

Comment on attachment 664205 [details] [diff] [review] Patch But ok, let's take this. Could you file a followup to get the documentation right or fix the callers.

(In reply to Olli Pettay [:smaug] from comment #21) > Comment on attachment 664205 [details] [diff] [review] > Patch > > But ok, let's take this. > Could you file a followup to get the documentation right or > fix the callers. I think moving to ShouldProcess() fixes this, since the ShouldProcess() doesn't require aContentLocation to be non-null: https://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIContentPolicy.idl#216 Would you like me to move the code to ShouldProcess or file the followup?

checking if aContentLocation is null in ShouldProcess(), before we call ShouldLoad().

(In reply to Tanvi Vyas from comment #22) > (In reply to Olli Pettay [:smaug] from comment #21) > > Comment on attachment 664205 [details] [diff] [review] > > Patch > > > > But ok, let's take this. > > Could you file a followup to get the documentation right or > > fix the callers. > > I think moving to ShouldProcess() fixes this, since the ShouldProcess() > doesn't require aContentLocation to be non-null: > https://mxr.mozilla.org/mozilla-central/source/content/base/public/ > nsIContentPolicy.idl#216 > > Would you like me to move the code to ShouldProcess or file the followup?

Ah, move the check to ShouldProcess then.

Comment on attachment 664228 [details] [diff] [review] null check in ShouldProcess() Carrying over r+

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

Try looks good. Can we land this patch?

pushed to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/e0dbda5b3ac4