1258111 - crash in nsContentPolicy::CheckPolicy

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-1988d519-9a26-412c-99fa-2989b2160312.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsContentPolicy::CheckPolicy(nsresult ( nsIContentPolicy::*)(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*), nsresult ( nsISimpleContentPolicy::*)(unsigned int, nsIURI*, nsIURI*, nsIDOMElement*, bool, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*), unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) 	dom/base/nsContentPolicy.cpp
1 	xul.dll 	nsContentPolicy::ShouldLoad(unsigned int, nsIURI*, nsIURI*, nsISupports*, nsACString_internal const&, nsISupports*, nsIPrincipal*, short*) 	dom/base/nsContentPolicy.cpp
2 	xul.dll 	NS_CheckContentLoadPolicy(unsigned int, nsIURI*, nsIPrincipal*, nsISupports*, nsACString_internal const&, nsISupports*, short*, nsIContentPolicy*, nsIScriptSecurityManager*) 	dom/base/nsContentPolicyUtils.h
3 	xul.dll 	nsFontFaceLoader::CheckLoadAllowed(nsIPrincipal*, nsIURI*, nsISupports*) 	layout/style/nsFontFaceLoader.cpp
4 	xul.dll 	mozilla::dom::FontFaceSet::CheckFontLoad(gfxFontFaceSrc const*, nsIPrincipal**, bool*) 	layout/style/FontFaceSet.cpp
5 	xul.dll 	gfxUserFontEntry::LoadNextSrc() 	gfx/thebes/gfxUserFontSet.cpp

this is a cross platform crash & many of the user comments seem to link this to printing a webpage.

Comment 1

[Nathan Froyd [:froydnj]]

Christoph, can you take a look at this?  It looks like we have no load context for a docshell, which is weird, or we have a window without a docshell, which is also kind of weird.  Perhaps we should be falling into the XUL document case below?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Nathan Froyd [:froydnj] from comment #1)
> Christoph, can you take a look at this?  It looks like we have no load
> context for a docshell, which is weird, or we have a window without a
> docshell, which is also kind of weird.  Perhaps we should be falling into
> the XUL document case below?

Bill, any chance I can pass this on to you? Within FF38 we introduced the crashing line of code [1]. It's odd that it hasn't been a problem for such a long time and now all of a sudden it is. Please also not that we converted Font loads to use asyncOpen2() within Bug 1195172, which landed in FF47. So maybe it's related to this change as well.

Potentially Froyd is right and we should just fall into the else branch underneath.

[1] https://hg.mozilla.org/mozilla-central/diff/9ba87714f5cf/dom/base/nsContentPolicy.cpp#l1.60

Comment 3

[[:philipp]]

curiously there was an increase of crashes with this signature yesterday (from a "normal" level of 10-50 daily crashes to over 800), but this mostly affected users of 46.0.1.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to [:philipp] from comment #3)
> curiously there was an increase of crashes with this signature yesterday
> (from a "normal" level of 10-50 daily crashes to over 800), but this mostly
> affected users of 46.0.1.

Philipp, can I get an URL (testcase) to reproduce the issue? Do we happen to have one handy?

Comment 5

[[:philipp]]

sorry, i won't be able to help with that as i cannot reproduce this issue myself (was only filing this based on looking at crash data). also as a volunteer i don't have access to url data submitted with crash reports...

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

There's basically one URL that dominates all the crashes. I don't want to post it since it includes users' email addresses. I wasn't able to figure out how to reproduce it myself though. Tracy, would you mind taking a look at this. Ping me if you need the URL, but I assume you have access. I'm not sure if it's the URL itself or some related part of the site. Also, it would be good to try different platforms I guess.

If that doesn't work, I guess we could add a null check. nsDocShell's implementation of QI for nsILoadInfo just does a simple cast, it looks like. So if the loadinfo is null, then the docshell must be null. But I don't know how a window can have no docshell. smaug, is that something that can happen?

Comment 7

[[:philipp]]

the top module correlation in yesterday's data from 46.0.1 also suggests that it is related to printing:
  nsContentPolicy::CheckPolicy|EXCEPTION_ACCESS_VIOLATION_READ (665 crashes)
    100% (665/665) vs.  36% (28391/78500) winspool.drv

Comment 8

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

Window is not guaranteed to have docshell if the docshell has been destroyed already.
something like:
var ifr = document.createElement("iframe");
document.body.appendChild(ifr);
var w = ifr.contentWindow;
var d = ifr.contentDocument;
document.body.removeChild(ifr);


... now do something with w or d...


As the method name "window->GetDocShell();" hints it may return null.
In DOM generally only methods without Get* prefix are guaranteed to never return null.

I don't know whether we're missing some null check in FontFaceSet code or in nsContentPolicy or both.

Comment 9

[Tracy Walker [:tracy]]

There were several Polish comments. I found one url that is a coupon in Polish probably sent out in an email promotion. I was able to reliably reproduce the crash when trying to print the coupon on Windows 7 and Mac 10.11.2 with release Fx47 as follows:

1) load the url for the coupon (let me know, if you don't have access to crash stat urls and I'll give you the exact url. if you do have access look for http://coupon.reserved.com/kupon_2016_06.html?campaign_id=209&email=<email snipped here>)
2) click the black button with the printer icon and "Wydrukuj kupon"
3) in the print dialog, just leave default options and print.

Crash

note: not able to reproduce with latest Nightly 50.0a1, 20160609030247

Comment 10

[Marco Castelluccio [:marco]]

(In reply to Tracy Walker [:tracy] from comment #9)
> There were several Polish comments. I found one url that is a coupon in
> Polish probably sent out in an email promotion. I was able to reliably
> reproduce the crash when trying to print the coupon on Windows 7 and Mac
> 10.11.2 with release Fx47 as follows:
> 
> 1) load the url for the coupon (let me know, if you don't have access to
> crash stat urls and I'll give you the exact url. if you do have access look
> for
> http://coupon.reserved.com/kupon_2016_06.html?campaign_id=209&email=<email
> snipped here>)
> 2) click the black button with the printer icon and "Wydrukuj kupon"
> 3) in the print dialog, just leave default options and print.
> 
> Crash
> 
> note: not able to reproduce with latest Nightly 50.0a1, 20160609030247

Can you reproduce if you save the page and open it locally? It would be great to attach a test case
to the bug so that we can test it easily (and we don't risk losing the test case when the coupon
expires).

Comment 11

[Tracy Walker [:tracy]]

The crash doesn't reproduce with a locally saved copy of the coupon page.  Let me dig deeper in the reported urls to see if I can find one that doesn't contain sensitive information that reliably crashes.

Comment 12

[Tracy Walker [:tracy]]

Hmmm, I tried many other urls and couldn't reproduce the crash.  (though many of them are login pages to sites, banks etc.)

The coupon that is dominating the volume of crashes in this signature expires June 12th.

Comment 13

[Marco Castelluccio [:marco]]

I've possibly found a fix using mozregression, bug 1262359.

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4b76e05f7ecf45a4a6877517f27c3d4d067802cd&tochange=e8dad24cfffa9f5d8406df8c59839902cd446e65

Comment 14

[Marco Castelluccio [:marco]]

Dismiss my last comment, unfortunately the bug is not 100% reproducible for me, so I had to rerun mozregression.

This time I found bug 1206961: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=632811bf4b6e96b54709d125c298493d30576eb8&tochange=bf69c3219b5ef75454640e9eaf087a2650c0c0eb

I can't be certain that the regression range is correct, as I had to try to print multiple times before hitting the crash (so in some cases I might have been "lucky" and incorrectly marked a build as "good").

I can only be sure of the "bad" builds. 53765e008b97622b5c7e26d22b2db016b0e53dc1 (2016-04-28) is bad.

Tracy, can you reproduce the bug consistently? If you do, can you try to find a fix with mozregression?

Comment 15

[Tracy Walker [:tracy]]

*sigh*  I'm now unable to reproduce with exact builds that I was reliable reproducing with this morning.  Using the exact same users coupon.  which make regression/fix hunting impossible.  :(

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Given the stacktrace, it's very likely that moving security checks into AsyncOpen2() within the FontLoader (Bug 1195172) has something to do with those crashes. Interesting however is that that conversion happenend within FF47. As phillip points out within comment 3, the most crashes we get are within FF46 though.

Even when looking through the changeset for converting the fontloader to rely on asyncOpen2() [1] there is no obvious change that would trigger the crash.

Maybe it's a website that changed into something explained within comment 8. I suppose moving this bug forward is to do what Nathan suggests within Comment 1 and actually add some kind of null check (or fall into the else branch if we can't query a nsILoadContent ) somewhere here [2].

[1] https://hg.mozilla.org/mozilla-central/rev/7251e33ee977
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentPolicy.cpp#162

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Bill, is this still on your radar or do we need someone else to fix that bug?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1258111_crash_in_checkpolicy.patch

Hey Bill, given this is one of the top crashes I think we should move forward and just add the simplest fix (nullcheck) which we can uplift to all the channels. If this needs a better fix, we can/should file a follow up bug. Agreed?

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8764485 [details] [diff] [review]
bug_1258111_crash_in_checkpolicy.patch

Approval Request Comment
According to comment 0, one of the top crashes.

[Feature/regressing bug #]:
Bug 1128798

[User impact if declined]:
Users visiting certain sites may experience a crash.

[Describe test coverage new/current, TreeHerder]:
Manually tested.

[Risks and why]: 
Low, just adding a null check.

[String/UUID change made/needed]:
no

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed9514f571d
Fix crash in CheckPolicy. r=billm

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9ed9514f571d

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8764485 [details] [diff] [review]
bug_1258111_crash_in_checkpolicy.patch

Top crash, taking it.
Should be in 48 beta 4

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f210e5d1a38d

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f210e5d1a38d

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/80579f7f434f

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'nsContentPolicy::CheckPolicy':
 - nightly (version 50): 0 crash from 2016-06-06.
 - aurora  (version 49): 1 crash from 2016-06-07.
 - beta    (version 48): 66 crashes from 2016-06-06.
 - release (version 47): 1494 crashes from 2016-05-31.
 - esr     (version 45): 262 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          0          0          0          0
 - aurora           0          0          0          0          0          1          0
 - beta             2          1          0         17         16          4         26
 - release        107         94        107         93        116         79        852
 - esr             13         16         14          9         18         15        133

Affected platforms: Windows, Mac OS X, Linux