crash in nsContentPolicy::CheckPolicy

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Security
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: philipp, Assigned: ckerschb)

Tracking

({crash})

46 Branch
mozilla50
x86
Windows NT
crash
Points:
---

Firefox Tracking Flags

(firefox45 wontfix, firefox46 wontfix, firefox47 wontfix, firefox48 fixed, firefox49 fixed, firefox-esr45 wontfix, firefox50 fixed)

Details

(Whiteboard: [domsecurity-active], crash signature)

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
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?
Component: General → DOM: Security
Flags: needinfo?(ckerschb)
(Assignee)

Comment 2

2 years ago
(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
Flags: needinfo?(ckerschb) → needinfo?(wmccloskey)
(Reporter)

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
(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?
Flags: needinfo?(madperson)
(Reporter)

Comment 5

2 years ago
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...
Flags: needinfo?(madperson)
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?
Flags: needinfo?(wmccloskey)
Flags: needinfo?(twalker)
Flags: needinfo?(bugs)
(Reporter)

Comment 7

2 years ago
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

2 years ago
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.
Flags: needinfo?(bugs)
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
Flags: needinfo?(twalker)
(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).
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.
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.
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?
Flags: needinfo?(twalker)
*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.  :(
Flags: needinfo?(twalker)
(Assignee)

Comment 16

2 years ago
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
(Assignee)

Comment 17

2 years ago
Bill, is this still on your radar or do we need someone else to fix that bug?
Flags: needinfo?(wmccloskey)
(Assignee)

Updated

2 years ago
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [domsecurity-active]
(Assignee)

Updated

2 years ago
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 18

2 years ago
Created attachment 8764485 [details] [diff] [review]
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?
Attachment #8764485 - Flags: review?(wmccloskey)
Attachment #8764485 - Flags: review?(wmccloskey) → review+
(Assignee)

Updated

2 years ago
Blocks: 1128798
(Assignee)

Comment 19

2 years ago
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
Attachment #8764485 - Flags: approval-mozilla-beta?
Attachment #8764485 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 20

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed9514f571d
Fix crash in CheckPolicy. r=billm
Keywords: checkin-needed

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ed9514f571d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
status-firefox45: affected → wontfix
status-firefox46: affected → wontfix
status-firefox47: affected → wontfix
status-firefox48: --- → affected
status-firefox49: --- → affected
Comment on attachment 8764485 [details] [diff] [review]
bug_1258111_crash_in_checkpolicy.patch

Top crash, taking it.
Should be in 48 beta 4
Attachment #8764485 - Flags: approval-mozilla-beta?
Attachment #8764485 - Flags: approval-mozilla-beta+
Attachment #8764485 - Flags: approval-mozilla-aurora?
Attachment #8764485 - Flags: approval-mozilla-aurora+

Comment 23

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f210e5d1a38d
status-firefox49: affected → fixed

Comment 25

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/80579f7f434f
status-firefox48: affected → fixed
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
status-firefox-esr45: --- → affected
status-firefox-esr45: affected → wontfix
You need to log in before you can comment on or make changes to this bug.