Last Comment Bug 749186 - crash in nsFontInflationData::FindFontInflationDataFor at crash address 0x28 (((nsIFrame*)0)->GetStateBits())
: crash in nsFontInflationData::FindFontInflationDataFor at crash address 0x28 ...
Status: RESOLVED FIXED
[native-crash][readability]
: crash, mobile, regression, reproducible, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 14 Branch
: ARM Android
: P1 critical (vote)
: mozilla16
Assigned To: Scott Johnson (:jwir3)
:
Mentors:
http://bit.ly/IHL5V3
: 761748 (view as bug list)
Depends on:
Blocks: 766599 747720 756999 763702
  Show dependency treegraph
 
Reported: 2012-04-26 07:34 PDT by Scoobidiver (away)
Modified: 2012-09-24 07:41 PDT (History)
15 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
betaN+


Attachments
b749186 (1.08 KB, patch)
2012-04-30 12:23 PDT, Scott Johnson (:jwir3)
dbaron: review-
Details | Diff | Splinter Review
b749186 (v2) (4.13 KB, patch)
2012-04-30 15:21 PDT, Scott Johnson (:jwir3)
dbaron: review-
Details | Diff | Splinter Review
b749186 (v3) (3.57 KB, patch)
2012-04-30 16:11 PDT, Scott Johnson (:jwir3)
dbaron: review+
mark.finkle: approval‑mozilla‑beta+
Details | Diff | Splinter Review
video of the crash (3.74 MB, video/webm)
2012-05-08 17:36 PDT, Kevin Brosnan [:kbrosnan]
no flags Details
testcase (uses enhanced privileges) (684 bytes, text/html)
2012-05-10 04:48 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase (uses SpecialPowers api) (522 bytes, text/html)
2012-05-10 04:50 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
b749186-followup1 (2.29 KB, patch)
2012-05-16 14:55 PDT, Scott Johnson (:jwir3)
dbaron: review-
Details | Diff | Splinter Review
bug749186-followup1 (3.57 KB, patch)
2012-05-24 13:32 PDT, Scott Johnson (:jwir3)
no flags Details | Diff | Splinter Review
b749186-crashtest (2.36 KB, patch)
2012-05-24 13:33 PDT, Scott Johnson (:jwir3)
dbaron: review+
Details | Diff | Splinter Review
bug749186-followup1 (v2) (3.73 KB, patch)
2012-05-25 10:52 PDT, Scott Johnson (:jwir3)
dbaron: review+
Details | Diff | Splinter Review
b749186-followup1 (v2) (4.92 KB, patch)
2012-06-01 16:02 PDT, Scott Johnson (:jwir3)
no flags Details | Diff | Splinter Review
b749186-followup-cached (14.89 KB, patch)
2012-06-05 16:54 PDT, Scott Johnson (:jwir3)
dbaron: review+
Details | Diff | Splinter Review
b749186-followup-cached (v2) (10.09 KB, patch)
2012-06-05 20:43 PDT, Scott Johnson (:jwir3)
jaywir3: review+
blassey.bugs: approval‑mozilla‑aurora+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Scoobidiver (away) 2012-04-26 07:34:41 PDT
It first appeared in 14.0a1/20120419.

Signature 	nsFontInflationData::FindFontInflationDataFor More Reports Search
UUID	9608fda3-fb96-4d43-8061-dae702120426
Date Processed	2012-04-26 13:50:50
Uptime	70
Last Crash	1.2 minutes before submission
Install Age	21.5 minutes since version was first installed.
Install Time	2012-04-26 13:28:42
Product	FennecAndroid
Version	14.0a2
Build ID	20120426042004
Release Channel	aurora
OS	Linux
OS Version	0.0.0 Linux 2.6.35.10-g9c2e87e #1 PREEMPT Thu Jul 7 11:31:33 CST 2011 armv7l
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0x28
App Notes 	
EGL? EGL+ AdapterVendorID: flyer, AdapterDeviceID: HTC Flyer P510e.
AdapterDescription: 'Android, Model: 'HTC Flyer P510e', Product: 'htc_flyer', Manufacturer: 'HTC', Hardware: 'flyer''.
GL Context? GL Context+ GL Layers? GL Layers+ 
HTC HTC Flyer P510e
htc_wwe/htc_flyer/flyer:2.3.4/GRJ22/102751.4:user/release-keys
Processor Notes 	This dump is too long and has triggered the automatic truncation routine
EMCheckCompatibility	True

Frame 	Module 	Signature 	Source
0 	libxul.so 	nsFontInflationData::FindFontInflationDataFor 	layout/generic/nsIFrame.h:1361
1 	libxul.so 	ShouldInflateFontsForContainer 	layout/base/nsLayoutUtils.cpp:4742
2 	libxul.so 	nsLayoutUtils::InflationMinFontSizeFor 	layout/base/nsLayoutUtils.cpp:4795
3 	libxul.so 	nsLayoutUtils::FontSizeInflationFor 	layout/base/nsLayoutUtils.cpp:4846
4 	libxul.so 	nsHTMLReflowState::CalcLineHeight 	layout/generic/nsHTMLReflowState.cpp:2237
5 	libxul.so 	nsBlockReflowState::nsBlockReflowState 	layout/generic/nsBlockReflowState.cpp:149
6 	libxul.so 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:1024
7 	libxul.so 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:295
8 	libxul.so 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3202
9 	libxul.so 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2511
10 	libxul.so 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:2022
11 	libxul.so 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:1071
12 	libxul.so 	nsBlockReflowContext::ReflowBlock 	layout/generic/nsBlockReflowContext.cpp:295
13 	libxul.so 	nsBlockFrame::ReflowBlockFrame 	layout/generic/nsBlockFrame.cpp:3202
14 	libxul.so 	nsBlockFrame::ReflowLine 	layout/generic/nsBlockFrame.cpp:2511
15 	libxul.so 	nsBlockFrame::ReflowDirtyLines 	layout/generic/nsBlockFrame.cpp:2022
16 	libxul.so 	nsBlockFrame::Reflow 	layout/generic/nsBlockFrame.cpp:1071
17 	libxul.so 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:941
18 	libxul.so 	nsCanvasFrame::Reflow 	layout/generic/nsCanvasFrame.cpp:458
19 	libxul.so 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:941
20 	libxul.so 	nsHTMLScrollFrame::ReflowScrolledFrame 	layout/generic/nsGfxScrollFrame.cpp:561
21 	libxul.so 	nsHTMLScrollFrame::ReflowContents 	layout/generic/nsGfxScrollFrame.cpp:659
22 	libxul.so 	nsHTMLScrollFrame::Reflow 	layout/generic/nsGfxScrollFrame.cpp:900
23 	libxul.so 	nsContainerFrame::ReflowChild 	layout/generic/nsContainerFrame.cpp:941
24 	libxul.so 	ViewportFrame::Reflow 	layout/generic/nsViewportFrame.cpp:231
25 	libxul.so 	PresShell::DoReflow 	layout/base/nsPresShell.cpp:7568
26 	libxul.so 	PresShell::ProcessReflowCommands 	layout/base/nsPresShell.cpp:7715
27 	libxul.so 	PresShell::FlushPendingNotifications 	layout/base/nsPresShell.cpp:4009
28 	libxul.so 	nsDocument::FlushPendingNotifications 	content/base/src/nsDocument.cpp:6383
29 	libxul.so 	nsGenericElement::GetPrimaryFrame 	content/base/src/nsGenericElement.cpp:3966
30 	libxul.so 	nsGenericElement::GetStyledFrame 	content/base/src/nsGenericElement.cpp:2047
31 	libxul.so 	nsGenericHTMLElement::GetOffsetRect 	content/html/content/src/nsGenericHTMLElement.cpp:502
32 	libxul.so 	nsGenericHTMLElement::GetOffsetWidth 	content/html/content/src/nsGenericHTMLElement.cpp:634
33 	libxul.so 	nsIDOMHTMLElement_GetOffsetWidth 	obj-firefox/js/xpconnect/src/dom_quickstubs.cpp:14449
34 	libxul.so 	js::GetPropertyHelper 	js/src/jscntxtinlines.h:364
35 	libxul.so 	js::Interpret 	js/src/jsinterpinlines.h:266
36 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:475
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsFontInflationData%3A%3AFindFontInflationDataFor
Comment 1 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-04-26 12:20:21 PDT
URL:
about:blank
about:config
http://news.ycombinator.com/item?id=3880213 
http://www.tumblr.com/dashboard
Comment 2 Scott Johnson (:jwir3) 2012-04-30 12:23:34 PDT
Created attachment 619637 [details] [diff] [review]
b749186

Added a null check to (hopefully) fix the crash. This seems to be what is causing it, but I can't reproduce, so I'm not 100% sure. Note that if FlowRootFor returns null, it's going to hit the assertion in nsFontInflationData::FindFontInflationDataFor(), but at least it won't crash like it is right now.

Since I can't reproduce, I am having difficulty writing a crashtest. :(
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-04-30 13:10:39 PDT
So the 0x28 crash address and the line number given for frame 0 are both consistent with the crash being a call to GetStateBits() on a null frame.  Still not sure how this would happen, though.
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-04-30 13:17:00 PDT
Comment on attachment 619637 [details] [diff] [review]
b749186

So the problem with this patch is that it's not going to help; the only caller of FlowRootFor assumes that the return value is non-null.

Looking at nsFrame::Init, I see two reasons the root frame might not be marked as a flow root:
 * font size inflation isn't enabled for the pres context (though I think it's pretty clear this isn't the problem since if this were the case nsLayoutUtils::FontSizeInflationFor would have returned early and wouldn't have called nsLayoutUtils::InflationMinFontSizeFor)
 * IsFontSizeInflationContainer returned false

I think one thing we could do here is give IsFontSizeInflationContainer a short-circuit return at the top that makes it always return true for the root frame.  This would actually simplify the code since we could remove the null-checks elsewhere.  It's possible that we're crashing in cases where the root has been explicitly styled as display:inline, or something like that.
Comment 5 Scott Johnson (:jwir3) 2012-04-30 15:21:45 PDT
Created attachment 619716 [details] [diff] [review]
b749186 (v2)

(In reply to David Baron [:dbaron] from comment #4)
> Comment on attachment 619637 [details] [diff] [review]
> b749186
> I think one thing we could do here is give IsFontSizeInflationContainer a
> short-circuit return at the top that makes it always return true for the
> root frame.  This would actually simplify the code since we could remove the
> null-checks elsewhere.  It's possible that we're crashing in cases where the
> root has been explicitly styled as display:inline, or something like that.

In this latest patch, this is the approach I took. I'm not sure if I found all of the null checks to remove, though. I'm also not quite sure I'm using the correct IsRootFrame() function, but I think it's correct. It just needed to be moved from the .cpp file into the .h file.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-04-30 15:45:21 PDT
Comment on attachment 619716 [details] [diff] [review]
b749186 (v2)

Just use !aFrame->GetParent() to see if it's the root frame.

And as far as removing null checks, I was actually thinking of the ones in IsFontSizeInflationContainer (checking aFrame->GetParent()), but the ones you removed are good too.

Other than that I think this looks good, but I probably want to look at it one more time.

Also, local style has a space between "if" and "(".
Comment 7 Scott Johnson (:jwir3) 2012-04-30 16:11:28 PDT
Created attachment 619739 [details] [diff] [review]
b749186 (v3)

(In reply to David Baron [:dbaron] from comment #6)
> Comment on attachment 619716 [details] [diff] [review]
> b749186 (v2)
> 
> Just use !aFrame->GetParent() to see if it's the root frame.
Ok, changed.

> And as far as removing null checks, I was actually thinking of the ones in
> IsFontSizeInflationContainer (checking aFrame->GetParent()), but the ones
> you removed are good too.
Ok, I added the ones in IsFontSizeInflationContainer.

> Also, local style has a space between "if" and "(".
Added a space.

> Other than that I think this looks good, but I probably want to look at it
> one more time.
Sounds good, posting it for review again.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-04-30 16:28:49 PDT
Comment on attachment 619739 [details] [diff] [review]
b749186 (v3)

r=dbaron
Comment 9 Scott Johnson (:jwir3) 2012-05-01 08:14:02 PDT
Baking on try:
https://hg.mozilla.org/try/rev/1f14b41d6a9b
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-03 12:06:42 PDT
looks like comment 9 accidentally reset the flags, so back to tracking with them!
Comment 11 Scott Johnson (:jwir3) 2012-05-04 10:25:34 PDT
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeba341b7e45
Comment 12 :Ms2ger (⌚ UTC+1/+2) 2012-05-05 03:40:57 PDT
https://hg.mozilla.org/mozilla-central/rev/eeba341b7e45
Comment 13 Scott Johnson (:jwir3) 2012-05-07 15:56:03 PDT
Comment on attachment 619739 [details] [diff] [review]
b749186 (v3)

[Approval Request Comment]
Regression caused by (bug #): 706193
User impact if declined: A small number of crashes
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk, We changed a very small set of code to verify a null pointer never happens. We also removed some null checks, since they should no longer be necessary - this would be the only source of risk. 
String changes made by this patch: none
Comment 14 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-08 12:05:58 PDT
https://crash-stats.mozilla.com/report/index/afec47f1-9c4e-47cd-92fd-a81052120507
Comment 15 Johnathan Nightingale [:johnath] 2012-05-08 12:08:29 PDT
(Didn't back anything out, but REOP anyhow to get this back onto the active blocker list)

This doesn't seem to be fixed.

https://crash-stats.mozilla.com/report/index/afec47f1-9c4e-47cd-92fd-a81052120507

That crash report is from the 20120507030511 nightly, which should post-date this commit, right?
Comment 16 Scott Johnson (:jwir3) 2012-05-08 13:08:15 PDT
(In reply to Johnathan Nightingale [:johnath] from comment #15)
> That crash report is from the 20120507030511 nightly, which should post-date
> this commit, right?

Yes, so apparently our assertion of what was causing the crash was incorrect. I'll take another look at it.
Comment 17 Kevin Brosnan [:kbrosnan] 2012-05-08 17:36:12 PDT
Created attachment 622230 [details]
video of the crash

I seem to get this crash going from uninflated text to inflated text. 

Open http://www.cnn.com/2012/05/08/showbiz/gallery/met-ball-2012/index.html
press the menu button > settings > text size > tiny
Back button
press the menu button > settings > text size > small
Boom (crash)

Galaxy SII Today's nightly
Comment 18 Kevin Brosnan [:kbrosnan] 2012-05-08 17:37:59 PDT
Missed a step

Open http://www.cnn.com/2012/05/08/showbiz/gallery/met-ball-2012/index.html
press the menu button > settings > text size > tiny
Back button
press the menu button > settings > text size > small
Back button
Boom (crash)
Comment 19 Scott Johnson (:jwir3) 2012-05-08 18:05:16 PDT
Awesome. I'll look into that tomorrow. Thanks for the STR.
Comment 20 Scott Johnson (:jwir3) 2012-05-09 09:27:31 PDT
(In reply to Kevin Brosnan [:kbrosnan] from comment #18)
> Missed a step
> 
> Open http://www.cnn.com/2012/05/08/showbiz/gallery/met-ball-2012/index.html
> press the menu button > settings > text size > tiny
> Back button
> press the menu button > settings > text size > small
> Back button
> Boom (crash)

Hmm... I can't seem to reproduce with either the current nightly or a personally compiled trunk version of fennec. You're able to reproduce this crash consistently?

I also have a Galaxy S2, so this is strange that I'm not seeing it...
Comment 21 Scott Johnson (:jwir3) 2012-05-09 09:28:01 PDT
Whoops. Set the flag accidentally. 

Also, what version of android do you have on your phone? Are you using ICS?
Comment 22 Scott Johnson (:jwir3) 2012-05-09 12:48:36 PDT
Shortened link:
http://bit.ly/IHL5V3
Comment 23 Scott Johnson (:jwir3) 2012-05-09 13:25:20 PDT
This just happened for me when I was investigating bug 752947. I was on the page discussed in that bug, and then I tried to reset font.size.inflation.emPerLine from 0 to 8 (I was on desktop firefox, linux), and I got the crash:
https://crash-stats.mozilla.com/report/index/dc538dce-4158-4e6d-aebc-3e56f2120509
Comment 24 Alex Keybl [:akeybl] 2012-05-09 15:54:57 PDT
We are leaving all non-beta+ bugs nominated for Aurora approval in the queue until FN14 Beta 1 is signed off on by QA.
Comment 25 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-10 04:48:43 PDT
Created attachment 622687 [details]
testcase (uses enhanced privileges)

This crashes current trunk build for me. The testcase uses enhanced privileges, for which the UI was nuked a couple of days ago. I'll also attach a testcase that uses the SpecialPowers api.

https://crash-stats.mozilla.com/report/index/bp-99bc79e1-b0e1-418d-a09b-f40722120510
Comment 26 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-10 04:50:19 PDT
Created attachment 622688 [details]
testcase (uses SpecialPowers api)

To reproduce with this testcase, install this extension:
http://people.mozilla.org/~mwargers/extensions/specialpowers.xpi
Comment 27 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-11 12:22:20 PDT
Comment on attachment 619739 [details] [diff] [review]
b749186 (v3)

Renom for the next patch
Comment 28 Scott Johnson (:jwir3) 2012-05-16 12:55:37 PDT
Ok, I think I see what's going on here. When the root frame is initialized, font.size.inflation.minTwips and font.size.inflation.emPerLine are both 0. They are later changed, but the state bits are set in Init(), so while font inflation is disabled initially, it's not disabled when this check is run, but the state bits weren't set, so it continues up to a null frame.

So, I think we need to perform the check (and possibly set state bits) in nsFontInflationData::FlowRootFor() instead of in nsFrame::Init().
Comment 29 Scott Johnson (:jwir3) 2012-05-16 13:30:11 PDT
(In reply to Scott Johnson (:jwir3) from comment #28)
> So, I think we need to perform the check (and possibly set state bits) in
> nsFontInflationData::FlowRootFor() instead of in nsFrame::Init().

Actually, on second thought, I think what we should do is set the state bits in nsFrame::Init() unconditionally (this is what happens in Debug mode anyway). That way, if font inflation _is_ enabled, the font inflation flow roots are already set. If it's not enabled, it's not going to hurt to have these bits set.
Comment 30 Scott Johnson (:jwir3) 2012-05-16 14:55:57 PDT
Created attachment 624552 [details] [diff] [review]
b749186-followup1

Followup that sets the font inflation state bits on frames, whether or not font inflation is enabled at frame initialization.
Comment 31 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-17 12:22:39 PDT
This seems to be adding a good bit of performance cost where we don't need it.

Instead, I think we should either:
 (a) cache whether inflation is enabled at the time of pres context creation, or
 (b) reconstruct the frame tree when the inflation prefs change
Comment 32 Scott Johnson (:jwir3) 2012-05-18 10:38:29 PDT
(In reply to David Baron [:dbaron] from comment #31)
> This seems to be adding a good bit of performance cost where we don't need
> it.

Are you concerned simply because the function IsFontSizeInflationContainer() is being called on every frame during initialization? It doesn't seem to me that this function adds a lot of performance impact, but I could be looking at it wrong. In debug builds, it was already going through this calculation during every call of nsFrame::Init() anyway. 

> Instead, I think we should either:
>  (a) cache whether inflation is enabled at the time of pres context
If we do this and font inflation is disabled at the time of pres context construction, then it is later enabled, won't we have to re-traverse the entire tree anyway, to determine where the font inflation roots are?

> creation, or
>  (b) reconstruct the frame tree when the inflation prefs change
I could go this route, but it actually seems less performant to me to do this than the patch I originally posted - that said, I could be missing something.
Comment 33 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-18 11:15:54 PDT
(In reply to Scott Johnson (:jwir3) from comment #32)
> (In reply to David Baron [:dbaron] from comment #31)
> > This seems to be adding a good bit of performance cost where we don't need
> > it.
> 
> Are you concerned simply because the function IsFontSizeInflationContainer()
> is being called on every frame during initialization? It doesn't seem to me
> that this function adds a lot of performance impact, but I could be looking
> at it wrong. In debug builds, it was already going through this calculation
> during every call of nsFrame::Init() anyway. 

I'm concerned about:
 * the performance cost of IsFontSizeInflationContainer().
 * the cost of nsFontInflationData::UpdateFontInflationDataWidthFor called from nsHTMLReflowState::Init
 * (there might be something else that's triggered by the bits being set, but maybe not)

> > Instead, I think we should either:
> >  (a) cache whether inflation is enabled at the time of pres context
> If we do this and font inflation is disabled at the time of pres context
> construction, then it is later enabled, won't we have to re-traverse the
> entire tree anyway, to determine where the font inflation roots are?

The point here would be that we wouldn't dynamically respond to changes in the inflation pref until the page is reloaded.  I don't think that's a serious problem.

> > creation, or
> >  (b) reconstruct the frame tree when the inflation prefs change
> I could go this route, but it actually seems less performant to me to do
> this than the patch I originally posted - that said, I could be missing
> something.

Changing the preference is an edge case whose performance we shouldn't worry about.  Loading every Web page in desktop firefox is not an edge case.
Comment 34 Scott Johnson (:jwir3) 2012-05-21 10:07:02 PDT
Ok, quick update for those following along at home:

I've decided to try both of these approaches, hopefully to see which one is better:

(In reply to David Baron [:dbaron] from comment #33)
> (a) cache whether inflation is enabled at the time of pres context
> creation, or

To do this, I'm going to remove the static variables from nsLayoutUtils that are set when the preferences are modified, and place them as member variables in nsPresShell. When nsPresShell is initialized, it'll read these preferences and set its appropriate member variables.

> (b) reconstruct the frame tree when the inflation prefs change
I added another option to nsPresContext::PreferenceChanged that detects when the font inflation preferences change, and then re-generates the frame tree.

I think option (a) is going to be better, but I'm not 100% sure.
Comment 35 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-24 10:21:45 PDT
I also crashed once with this stacktrace (or similar one) when the viewport of the device was changed (portrait to landscape), btw.
Comment 36 Scott Johnson (:jwir3) 2012-05-24 13:32:57 PDT
Created attachment 626944 [details] [diff] [review]
bug749186-followup1

Patch that regenerates the frame tree when these preferences change.
Comment 37 Scott Johnson (:jwir3) 2012-05-24 13:33:36 PDT
Created attachment 626946 [details] [diff] [review]
b749186-crashtest

Mochitest that should trigger a crash for this bug.
Comment 38 Chris Peterson [:cpeterson] 2012-05-25 10:35:49 PDT
This crash is currently Fennec 15.0a1's #3 topcrash for the past 3 days.
Comment 39 Scott Johnson (:jwir3) 2012-05-25 10:52:52 PDT
Created attachment 627290 [details] [diff] [review]
bug749186-followup1 (v2)

Slightly modified the code in the UpdateAfterPreferencesChanged handler to get font-inflation tests to pass without assertions.
Comment 40 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-25 17:04:48 PDT
Comment on attachment 627290 [details] [diff] [review]
bug749186-followup1 (v2)

>+  if (StringBeginsWith(prefName, NS_LITERAL_CSTRING("font.size.inflation"))) {

Stick a "." at the end of the string.


>   nsChangeHint hint = nsChangeHint(0);
> 
>-  if (mPrefChangePendingNeedsReflow) {
>+  if (mPrefChangePendingNeedsReframe) {
>+    nsIPresShell* presShell = GetPresShell();
>+    if (presShell) {
>+      nsIFrame* rootFrame = presShell->GetRootFrame();
>+      if (rootFrame) {
>+        if (rootFrame->GetContent()) {
>+          presShell->RecreateFramesFor(rootFrame->GetContent());
>+        }
>+      }
>+    }
>+  } else if (mPrefChangePendingNeedsReflow) {
>     NS_UpdateHint(hint, NS_STYLE_HINT_REFLOW);
>+    RebuildAllStyleData(hint);
>   }
>-
>-  RebuildAllStyleData(hint);

Don't move the RebuildAllStyleData() call inside the Reflow case; we need it for when neither is set.

Second, handle the reframe the same way -- by setting the hint, which RebuildAllStyleData will use.  Call NS_UpdateHint(hint, nsChangeHint_ReconstructFrame).


r=dbaron with those changes
Comment 41 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-25 17:07:20 PDT
Comment on attachment 626946 [details] [diff] [review]
b749186-crashtest

>+     SimpleTest.waitForExplicitFinish();
>+     setTimeout(function() {SpecialPowers.setIntPref("font.size.inflation.emPerLine", 8)},100);
>+     setTimeout(function() {document.getElementById('b').removeAttribute('style');}, 200);
>+     setTimeout(function() {SpecialPowers.setIntPref("font.size.inflation.emPerLine", 0)}, 300);
>+     setTimeout(function() {ok(true, 'test finished without crashing');}, 700);
>+     setTimeout(function() {SimpleTest.finish(); }, 1000);

You should restructure this so that:
 * each timeout calls the next one when it's done its work, so that clock skew can't mess things up
 * you don't need to spend a whole second waiting for things -- force flushes if you need them, and setTimeout(..., 0) if you need to, but you really shouldn't need anything to wait to get this to crash

r=dbaron with those things fixed
Comment 42 Scott Johnson (:jwir3) 2012-05-27 10:37:11 PDT
(In reply to David Baron [:dbaron] from comment #40)
> Comment on attachment 627290 [details] [diff] [review]
> bug749186-followup1 (v2)
> 
> >+  if (StringBeginsWith(prefName, NS_LITERAL_CSTRING("font.size.inflation"))) {
> 
> Stick a "." at the end of the string.

Done.

> >   nsChangeHint hint = nsChangeHint(0);
> > 
> >-  if (mPrefChangePendingNeedsReflow) {
> >+  if (mPrefChangePendingNeedsReframe) {
> >+    nsIPresShell* presShell = GetPresShell();
> >+    if (presShell) {
> >+      nsIFrame* rootFrame = presShell->GetRootFrame();
> >+      if (rootFrame) {
> >+        if (rootFrame->GetContent()) {
> >+          presShell->RecreateFramesFor(rootFrame->GetContent());
> >+        }
> >+      }
> >+    }
> >+  } else if (mPrefChangePendingNeedsReflow) {
> >     NS_UpdateHint(hint, NS_STYLE_HINT_REFLOW);
> >+    RebuildAllStyleData(hint);
> >   }
> >-
> >-  RebuildAllStyleData(hint);
> 
> Don't move the RebuildAllStyleData() call inside the Reflow case; we need it
> for when neither is set.

Fixed.

> Second, handle the reframe the same way -- by setting the hint, which
> RebuildAllStyleData will use.  Call NS_UpdateHint(hint,
> nsChangeHint_ReconstructFrame).

I originally tried this method, but nsCSSFrameConstructor::RebuildAllStyleData has this assertion:
NS_ASSERTION(!(aExtraHint & nsChangeHint_ReconstructFrame),
               "Should not reconstruct the root of the frame tree.  "
               "Use ReconstructDocElementHierarchy instead.");

The way I did it will eventually call ReconstructDocElementHierarchy() instead of trying to do it from RebuildAllStyleData. 

Perhaps I'm missing something here?
Comment 43 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-27 10:57:07 PDT
(In reply to Scott Johnson (:jwir3) from comment #42)
> > Second, handle the reframe the same way -- by setting the hint, which
> > RebuildAllStyleData will use.  Call NS_UpdateHint(hint,
> > nsChangeHint_ReconstructFrame).
> 
> I originally tried this method, but
> nsCSSFrameConstructor::RebuildAllStyleData has this assertion:
> NS_ASSERTION(!(aExtraHint & nsChangeHint_ReconstructFrame),
>                "Should not reconstruct the root of the frame tree.  "
>                "Use ReconstructDocElementHierarchy instead.");
> 
> The way I did it will eventually call ReconstructDocElementHierarchy()
> instead of trying to do it from RebuildAllStyleData. 
> 
> Perhaps I'm missing something here?

In that case, you should just call presShell->ReconstructFrames() and then return early.  Don't bother with getting the root content node, and (by returning early) don't bother calling RebuildAllStyleData too.

However, I'm a little concerned that there might be a problem with it being synchronous, although I suppose it should be OK because the preference change handling code is already doing coalescing.
Comment 44 Scott Johnson (:jwir3) 2012-05-29 11:19:06 PDT
(In reply to David Baron [:dbaron] from comment #43)
> In that case, you should just call presShell->ReconstructFrames() and then
> return early.  Don't bother with getting the root content node, and (by
> returning early) don't bother calling RebuildAllStyleData too.
> 
> However, I'm a little concerned that there might be a problem with it being
> synchronous, although I suppose it should be OK because the preference
> change handling code is already doing coalescing.

I spoke with dbaron about this via Vidyo, and because there are some assertions that come up during font inflation unit tests (causing them to fail) when presShell->ReconstructFrames() is called, we decided to leave it as-is.
Comment 45 Scott Johnson (:jwir3) 2012-05-29 14:54:47 PDT
Just waiting for try results, then I will be pushing this to inbound -
https://tbpl.mozilla.org/?tree=Try&rev=266ff8a1e2b7
Comment 46 Scott Johnson (:jwir3) 2012-05-30 10:06:58 PDT
> However, I'm a little concerned that there might be a problem with it being
> synchronous, although I suppose it should be OK because the preference
> change handling code is already doing coalescing.

Hmm... so in order to test this, I restructured my test a little bit, and now I can get it to crash because the notification (and thus the reframe) happens at a later time than the reflow that expects the flow root to be set, probably happening from the fact that the preference notification is asynchronous. 

What I could do is place a notification function in nsLayoutUtils and register it using mozilla::Preferences::RegisterCallback. This seems to happen at the correct time. The problem is that since nsLayoutUtils is composed of static methods, I don't have a reference to the pres shell...

I'm still trying to work this out, but perhaps it might be better to go with the patch that caches the font inflation settings in the pres shell. I already have it ready to go, so I could post it for review almost immediately.

Thoughts, dbaron?
Comment 47 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-30 10:41:26 PDT
Can you just move the RecreateFrames call into PresContext::PreferenceChanged?

(Maybe there's a way to have it destroy frames immediately but not recreate them until later, though?  tn would probably know.)
Comment 48 Scott Johnson (:jwir3) 2012-05-30 13:04:44 PDT
(In reply to David Baron [:dbaron] from comment #47)
> Can you just move the RecreateFrames call into
> PresContext::PreferenceChanged?

Yeah, that's something I tried already, but it seems that this code is also called asynchronously, as the reflow seems to happen before the frame tree can be destroyed. (i.e. it doesn't fix the crash)

> (Maybe there's a way to have it destroy frames immediately but not recreate
> them until later, though?  tn would probably know.)

I just sent an email to him in the hopes that he might have some insight.
Comment 49 Scott Johnson (:jwir3) 2012-05-30 18:34:02 PDT
I had a thought this afternoon...

What if we separate out the logic that sets font inflation flow roots, i.e. this code in nsFrame.cpp:

    if (IsFontSizeInflationContainer(this, disp)) {
      AddStateBits(NS_FRAME_FONT_INFLATION_CONTAINER);
      if (!GetParent() ||
          // I'd use NS_FRAME_OUT_OF_FLOW, but it's not set yet.
          disp->IsFloating() || disp->IsAbsolutelyPositioned()) {
        AddStateBits(NS_FRAME_FONT_INFLATION_FLOW_ROOT);
      }
    }
    NS_ASSERTION(GetParent() ||
                 (GetStateBits() & NS_FRAME_FONT_INFLATION_CONTAINER),
                 "root frame should always be a container");
  }

and make this a separate function in nsLayoutUtils. Then, we could have another function, that, given a frame tree, traverses that tree and sets the font inflation containers and flow roots as necessary. We could call this if the preferences changed, or if we detected that we don't have a flow root somewhere where we should have one (this last part is a bit of a hack). That way, we wouldn't need to reconstruct the entire frame tree, and instead only need to set appropriate state bits. 

Thoughts?
Comment 50 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-30 22:44:14 PDT
Not unreasonable, but it still seems pretty odd that a synchronous frame reconstruct would leave a crash still happening.  Do you know why it's not soon enough?
Comment 51 Scott Johnson (:jwir3) 2012-05-31 11:10:56 PDT
(In reply to David Baron [:dbaron] from comment #50)
> Not unreasonable, but it still seems pretty odd that a synchronous frame
> reconstruct would leave a crash still happening.  Do you know why it's not
> soon enough?

Now I think I may have been incorrect. I think it _is_ getting called in the correct order, but for some reason, the state bits still aren't getting set properly.
Comment 52 Scott Johnson (:jwir3) 2012-05-31 12:56:28 PDT
Ok, I think I figured out what's happening here. It looks like, for some reason, the frame tree is being rebuilt correctly, but nsLayoutUtils::FontSizeInflationEnabled() is reporting false, because sFontSizeInflationEmPerLine and sFontSizeInflationMinTwips are both 0. 

When I check the values from the preferences directly, they are set to the expected values (in this case, emPerLine is 8). So, I wonder if the variable caching that's set up in nsLayoutUtils::Initialize() is happening after the reframe somehow...
Comment 53 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-31 13:30:09 PDT
(In reply to Scott Johnson (:jwir3) from comment #52)
> Ok, I think I figured out what's happening here. It looks like, for some
> reason, the frame tree is being rebuilt correctly, but
> nsLayoutUtils::FontSizeInflationEnabled() is reporting false, because
> sFontSizeInflationEmPerLine and sFontSizeInflationMinTwips are both 0. 
> 
> When I check the values from the preferences directly, they are set to the
> expected values (in this case, emPerLine is 8). So, I wonder if the variable
> caching that's set up in nsLayoutUtils::Initialize() is happening after the
> reframe somehow...

Ah, yes, that seems likely.  Maybe the code that does the reframe needs to re-retrieve those preferences first?
Comment 54 Scott Johnson (:jwir3) 2012-06-01 16:02:23 PDT
Created attachment 629384 [details] [diff] [review]
b749186-followup1 (v2)

Here's a new patch I wrote to hopefully address the issue that I discussed in Comment 52. However, there's still a slight problem. If I don't put the code into PreferenceChanged(), and instead use a flag mPreferenceChangedPendingReframe, and process it in UpdateAfterPreferencesChanged(), then I still get the crash (presumably due to a reflow happening between when the preferences are changed and when the reframe actually gets processed).

However, if I place it in PreferenceChanged(), then I get the following assertions when I run a couple of reftests in the font-inflation/ directory (this is from intrinsic-min-1.html, but it happens on video-1.html as well, as well as giving some leak notifications) (also, ignore the last few lines, those are from my wrapper script):

http://pastebin.mozilla.org/1653663
 
I could place asserts() before the reftests, but my intuition tells me there could be an actual problem here. I can't seem to figure out how the presShell is destroyed prematurely, though. Perhaps I'm actually leaking something that I should be holding a strong reference to?
Comment 55 Scoobidiver (away) 2012-06-03 09:17:30 PDT
It's #20 top crasher in 14.0b3 but #2 in 14.0b4 and 14.0b5. It spiked from 14.0a2/20120524. The regression range for the spike is:
http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?fromchange=cb1661106d95&tochange=125c4f8ff0da
It's likely a regression from bug 747720.
Comment 56 :Ehsan Akhgari 2012-06-03 09:57:25 PDT
I reproduced this crash by doing the following:

1. start Aurora, and as Gecko is loading, tap the location bar, enter "nightly" and tap on the Google search icon.
2. as the page is loading, Firefox crashes.  I can reproduce this pretty reliably, for example: https://crash-stats.mozilla.com/report/index/bp-4017c026-0164-404d-ba5f-20c4f2120603
Comment 57 :Ehsan Akhgari 2012-06-03 09:59:31 PDT
and in fact, it seems like this happens even after startup.
Comment 58 Scott Johnson (:jwir3) 2012-06-04 22:14:29 PDT
dbaron and I spoke this past afternoon, and we concluded that moving the code into the UpdateAfterPreferenceChanged() function was easier than trying to deal with the assertions. The problem faced here, though, is that the bug isn't fixed (i.e. there's still a crash, with call stack: http://pastebin.mozilla.org/1655635 ). 

dbaron recommended that I add a function that calls UpdateAfterPreferenceChanged from within PresShell::FlushPendingNotifications() if the has a pending notification. I did this, and the crash is still appearing. 

I've tracked this down now to verify that the following is happening (in order):

1) Preference for font inflation is changed
2) pres context is notified, sets reframe needed bit
3) Flush notifications is called from within the pres shell
4) UpdateAfterPreferenceChanged is called, using the new function described above
5) Reframe event happens successfully
6) Font inflation containers are correctly assigned, but FONT INFLATION FLOW ROOTS are not.
7) Expected font inflation flow root is not found, since one doesn't exist, and so the crash still happens.

#6 is a little fuzzy still to me how this happens. Since the following code exists in nsFrame's init function:

    NS_ASSERTION(GetParent() ||
                 (GetStateBits() & NS_FRAME_FONT_INFLATION_CONTAINER),
                 "root frame should always be a container");

and this assertion doesn't appear to be hit, it would seem to indicate that the root frame isn't getting reconstructed with the other frames...
Comment 59 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-04 22:29:59 PDT
So it's entirely possible that some or all the ways of reconstructing the frame tree don't actually reconstruct the root frame.  But if that was the case then none of the patches would have worked, I'd think.  (Or maybe not, if they reconstructed something close to the root frame that would also be a flow root, perhaps?)
Comment 60 Scott Johnson (:jwir3) 2012-06-04 22:38:32 PDT
(In reply to David Baron [:dbaron] from comment #59)
> So it's entirely possible that some or all the ways of reconstructing the
> frame tree don't actually reconstruct the root frame.  But if that was the
> case then none of the patches would have worked, I'd think.  (Or maybe not,
> if they reconstructed something close to the root frame that would also be a
> flow root, perhaps?)

Well, if I place this code into PreferenceChanged(), then it resolves the crash, but as I mentioned in IRC, it causes assertions that look pretty nasty.

BTW - the frame tree at the time of the crash (after the reframe event) looks like:

(gdb) pf $myVar->mParent->mParent->mParent
Viewport(-1)@0x7fffd4d960a0 [view=0x7fffc8696a00] {0,0,69480,48660} [state=0000002400003029] [content=(nil)] [sc=0x7fffc84b80d8] pst=:-moz-viewport<
  HTMLScroll(html)(-1)@0x7fffd4d96bb0 {0,0,0,0} [state=0000020000000403] [content=0x7fffc86b2c90] [sc=0x7fffd415e5d8] pst=:-moz-viewport-scroll<
    ScrollbarFrame(scrollbar)(-1)@0x7fffd4d970a8 next=0x7fffc84b5f60 {0,0,0,0} [state=0000100080c80402] [content=0x7fffc86f33e0] [sc=0x7fffc84b8e60]<
      ButtonBoxFrame(scrollbarbutton)(-1)@0x7fffc84b5020 next=0x7fffc84b52f8 {0,0,0,0} [state=2000160080c00402] [content=0x7fffc86f38f0] [sc=0x7fffc8695d20]<>
      SliderFrame(slider)(-1)@0x7fffc84b52f8 next=0x7fffc84b5c08 {0,0,0,0} [state=0000160080c00402] [content=0x7fffc86f3a10] [sc=0x7fffd4d96688]<
        ButtonBoxFrame(thumb)(0)@0x7fffc84b5928 {0,0,0,0} [state=2000160080400402] [content=0x7fffc86f3aa0] [sc=0x7fffd4d977e8]<>
      >
      ButtonBoxFrame(scrollbarbutton)(-1)@0x7fffc84b5c08 {0,0,0,0} [state=2000160080c00402] [content=0x7fffc86f3ce0] [sc=0x7fffd4d97580]<>
    >
    ScrollbarFrame(scrollbar)(-1)@0x7fffc84b5f60 next=0x7fffc84b7748 {0,0,0,0} [state=0000100080880402] [content=0x7fffc86f3590] [sc=0x7fffd4d97a98]<
      ButtonBoxFrame(scrollbarbutton)(-1)@0x7fffc84b6c18 next=0x7fffc84b6cc0 {0,0,0,0} [state=2000160080c00402] [content=0x7fffc86f3e00] [sc=0x7fffc84b6668]<>
      SliderFrame(slider)(-1)@0x7fffc84b6cc0 next=0x7fffc84b7258 {0,0,0,0} [state=0000160080800402] [content=0x7fffc86f40d0] [sc=0x7fffc84b8410]<
        ButtonBoxFrame(thumb)(0)@0x7fffc84b70a0 {0,0,0,0} [state=2000160080000402] [content=0x7fffc86f4160] [sc=0x7fffd4d97d00]<>
      >
      ButtonBoxFrame(scrollbarbutton)(-1)@0x7fffc84b7258 {0,0,0,0} [state=2000160080c00402] [content=0x7fffc86f4670] [sc=0x7fffc84b82c0]<>
    >
    Box(scrollcorner)(-1)@0x7fffc84b7748 next=0x7fffd4d968e8 {0,0,0,0} [state=0000100080c00402] [content=0x7fffc86f3860] [sc=0x7fffc84b5cf8]<>
    Canvas(html)(-1)@0x7fffd4d968e8 {0,0,68400,48660} [state=0000002000000403] [content=0x7fffc86b2c90] [sc=0x7fffd4d97288] pst=:-moz-scrolled-canvas<
      Block(html)(-1)@0x7fffc84b7cd0 [content=0x7fffc86b2c90] {0,0,0,0} [state=0000100000d00403] sc=0x7fffc84d31b0(i=0,b=1)<
        line 0x7fffc84b8598: count=1 state=block,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x9] {0,0,0,0} <
          Block(body)(2)@0x7fffc84b8500 [content=0x7fffc86f3620] {0,0,0,0} [state=0000120000100402] sc=0x7fffc84b7e80(i=1,b=0)<
            line 0x7fffc84b8b08: count=2 state=inline,dirty,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x41] {0,0,0,0} <
              Text(2)"\n"@0x7fffc84b8a20 [run=(nil)][0,1,T]  next=0x7fffc84b8a90 {0,0,0,0} [state=0000000000000402] [content=0x7fffc88a1380] sc=0x7fffc84b8360 pst=:-moz-non-element
              Inline(div)(3)@0x7fffc84b8a90 {0,0,0,0} [state=0000100000000402] [content=0x7fffc88a1400] [sc=0x7fffc84d4820]<>
            >
          >
        >
      >
    >
  >
>

where myVar is the frame for which we want a flow root, in this case:
(gdb) p $myVar
$6 = (nsBlockFrame *) 0x7fffc84b7cd0

and nothing exists above Viewport(-1)@0x7fffd4d960a0. That is to say, $myVar->mParent->mParent->mParent->mParent == 0x0.

Even if I change the assertion in nsFrame::Init to:
    NS_ASSERTION(GetParent() ||
                 (GetStateBits() & NS_FRAME_FONT_INFLATION_CONTAINER),
                 "root frame should always be a container");

It isn't hit, and this crash still happens. So, this seems to indicate that the root frame isn't being reframed as expected...
Comment 61 Timothy Nikkel (:tnikkel) 2012-06-04 22:40:03 PDT
(In reply to David Baron [:dbaron] from comment #59)
> So it's entirely possible that some or all the ways of reconstructing the
> frame tree don't actually reconstruct the root frame.

I could see this being the case. PresShell::ReconstructFrames calls nsCSSFrameConstructor::ReconstructDocElementHierarchy which calls RecreateFramesForContent(mDocument->GetRootElement()). The root element doesn't correspond to the root frame but rather a child of the root frame. The root frame is a viewport frame and has a null mContent pointer so I could see it not getting reconstructed.
Comment 62 Timothy Nikkel (:tnikkel) 2012-06-04 22:47:46 PDT
Yeah, it looks like the only time we currently reconstruct the root frame is if we reconstruct the whole presentation (so it's not a normal frame reconstruct).
Comment 63 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-04 23:02:47 PDT
Then I think we should make the code that marks the root frame as an inflation flow root and container be unconditional (simple fix to nsFrame::Init) -- just add an "|| !GetParent()" to the check for whether inflation is enabled.
Comment 64 Scoobidiver (away) 2012-06-05 12:56:36 PDT
*** Bug 761748 has been marked as a duplicate of this bug. ***
Comment 65 Scott Johnson (:jwir3) 2012-06-05 16:54:36 PDT
Created attachment 630372 [details] [diff] [review]
b749186-followup-cached

This is a workaround patch that caches font inflation preference values in the pres shell. The reason we're experiencing this crash is due to the frame construction happening when font inflation is turned off, and then font inflation being turned on, but frame state bits not being set (because they weren't set during frame construction).

This patch makes it so that once values are set, they are set until the pres shell is reconstructed (thus reconstruction of the frame tree with the appropriate state bits).
Comment 66 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-05 18:58:28 PDT
Comment on attachment 630372 [details] [diff] [review]
b749186-followup-cached

>   /**
>+   * Methods that change and retrieve the cached font inflation preferences.
>+   */
>+  void SetFontSizeInflationEmPerLine(PRUint32 aEmPerLine) {
>+    mFontSizeInflationEmPerLine = aEmPerLine;
>+  }
>+
>+  void SetFontSizeInflationMinTwips(PRUint32 aMinTwips) {
>+    mFontSizeInflationMinTwips = aMinTwips;
>+  }
>+
>+  void SetFontSizeInflationLineThreshold(PRUint32 aLineThreshold) {
>+    mFontSizeInflationLineThreshold = aLineThreshold;
>+  }

I'd skip these setters and just have PresShell::SetupFontInflation touch the member variables directly.

> /* static */ bool
> nsLayoutUtils::FontSizeInflationEnabled(nsPresContext *aPresContext)
> {
>-  if ((sFontSizeInflationEmPerLine == 0 &&
>-       sFontSizeInflationMinTwips == 0) ||
>+  nsIPresShell* presShell = aPresContext->GetPresShell();
>+
>+  if (!presShell || (presShell->FontSizeInflationEmPerLine() == 0 &&
>+       presShell->FontSizeInflationMinTwips() == 0) ||

put a line break after the first ||; otherwise it's weird to have the line breaks not match the grouping

>+void
>+PresShell::SetupFontInflation() {

{ on its own line

>+  SetFontSizeInflationEmPerLine(nsLayoutUtils::FontSizeInflationEmPerLine());
>+  SetFontSizeInflationMinTwips(nsLayoutUtils::FontSizeInflationMinTwips());
>+  SetFontSizeInflationLineThreshold(nsLayoutUtils::FontSizeInflationLineThreshold());

see above

> /* static */ void
>-nsFontInflationData::UpdateFontInflationDataWidthFor(const nsHTMLReflowState& aReflowState)
>+nsFontInflationData::UpdateFontInflationDataWidthFor(nsPresContext* aPresContext,
>+                                                     const nsHTMLReflowState& aReflowState)
> {
>   nsIFrame *bfc = aReflowState.frame;
>   NS_ASSERTION(bfc->GetStateBits() & NS_FRAME_FONT_INFLATION_FLOW_ROOT,
>                "should have been given a flow root");
>   FrameProperties bfcProps(bfc->Properties());
>   nsFontInflationData *data = static_cast<nsFontInflationData*>(
>                                 bfcProps.Get(FontInflationDataProperty()));
>   if (!data) {
>     data = new nsFontInflationData(bfc);
>     bfcProps.Set(FontInflationDataProperty(), data);
>   }
> 
>-  data->UpdateWidth(aReflowState);
>+  data->UpdateWidth(aPresContext, aReflowState);
> }


Don't add these parameters.

> void
>-nsFontInflationData::UpdateWidth(const nsHTMLReflowState &aReflowState)
>+nsFontInflationData::UpdateWidth(nsPresContext* aPresContext,
>+                                 const nsHTMLReflowState &aReflowState)

Ditto.

>-  PRUint32 lineThreshold = nsLayoutUtils::FontSizeInflationLineThreshold();
>+  nsIPresShell* presShell = aPresContext->PresShell();
>+  PRUint32 lineThreshold = presShell->FontSizeInflationLineThreshold();

Instead, here, use bfc->PresContext()->PresShell();

>diff --git a/layout/generic/nsFontInflationData.h b/layout/generic/nsFontInflationData.h

No new parameters.

>--- a/layout/generic/nsFontInflationData.h

Same.

>diff --git a/layout/generic/nsHTMLReflowState.cpp b/layout/generic/nsHTMLReflowState.cpp

Same.


r=dbaron with that
Comment 67 Scott Johnson (:jwir3) 2012-06-05 20:43:08 PDT
Created attachment 630432 [details] [diff] [review]
b749186-followup-cached (v2)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Large number of crashes when font inflation enabled.
Testing completed (on m-c, etc.): local testing only to this point
Risk to taking this patch (and alternatives if risky): Not risky. Simply caches preference values in the presShell. Very little code changed.
String or UUID changes made by this patch: none.
Comment 68 Brad Lassey [:blassey] (use needinfo?) 2012-06-05 20:55:13 PDT
Comment on attachment 630432 [details] [diff] [review]
b749186-followup-cached (v2)

[Triage Comment]
Comment 69 Scott Johnson (:jwir3) 2012-06-05 20:57:11 PDT
Landed on Beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/ec0841d4034e
Comment 70 Scott Johnson (:jwir3) 2012-06-05 21:42:27 PDT
Had a bad merge, and was backed out on beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/23f3e1dfaa6c
Comment 72 Scott Johnson (:jwir3) 2012-06-05 22:02:57 PDT
Pushed to beta again:
https://hg.mozilla.org/releases/mozilla-beta/rev/e835d6524759
Comment 73 Scott Johnson (:jwir3) 2012-06-05 23:32:13 PDT
And pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/86f07056bdc9
Comment 74 Scoobidiver (away) 2012-06-06 13:03:48 PDT
There are still crashes in 16.0a1/20120606 that contains the patch.
Comment 75 Scott Johnson (:jwir3) 2012-06-06 14:12:12 PDT
(In reply to Scoobidiver from comment #74)
> There are still crashes in 16.0a1/20120606 that contains the patch.

I can't see that on crash-stats... can you post me a link to where you can see that this is the case?
Comment 76 Scott Johnson (:jwir3) 2012-06-06 14:19:38 PDT
I can no longer reproduce this with Special Powers API installed and attachment 622688 [details]. Can you post STR?
Comment 77 Scoobidiver (away) 2012-06-07 00:52:00 PDT
(In reply to Scott Johnson (:jwir3) from comment #75)
> I can't see that on crash-stats... can you post me a link to where you can
> see that this is the case?
The link is at the end of comment 0.
Comment 78 Brad Lassey [:blassey] (use needinfo?) 2012-06-07 09:58:41 PDT
(In reply to Scott Johnson (:jwir3) from comment #75)
> (In reply to Scoobidiver from comment #74)
> > There are still crashes in 16.0a1/20120606 that contains the patch.
> 
> I can't see that on crash-stats... can you post me a link to where you can
> see that this is the case?

3 crashes on crash-stats for the 06/06 build off of changeset 6338a8988917 which has this patch
Comment 79 Jet Villegas (:jet) 2012-06-07 11:59:35 PDT
Please confirm if the STR still repros in the Beta build. If not, please find us a new URL to crash this.
Comment 80 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-07 15:06:52 PDT
The STRs do not seem to reproduce the issue.  I can't seem to reproduce with the following links either (these are links that are associated with the current crash)

Listed URLs: 

about:home
https://www.google.com/m?q=boot+to+gecko&ie=utf-8&oe=utf-8&aq=t&rls=org.mozilla:
https://www.google.com/m?q=%D0%BA%D0%BE%D0%BD%D1%82%D1%80%D0%B0%D0%BA%D1%82%D0%B
https://wiki.mozilla.org/Mobile/Notes/30-May-2012
Comment 81 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-08 14:28:28 PDT
Comment on attachment 619739 [details] [diff] [review]
b749186 (v3)

We never landed this on beta, and I think we should.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: potential crash
Testing completed (on m-c, etc.): on mozilla-central for over a month
Risk to taking this patch (and alternatives if risky):  low
String or UUID changes made by this patch: none
Comment 84 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-10 11:52:06 PDT
Maybe bug 760098?
Comment 85 Scoobidiver (away) 2012-06-11 01:09:42 PDT
(In reply to Scoobidiver from comment #83)
> Crashes stopped after 16.0a1/20120607 so far (I am crossing the fingers)
Unfortunately, there are two crashes in 16.0a1/20120609.
Comment 86 Robert Kaiser 2012-06-11 10:19:44 PDT
Is attachment 619739 [details] [diff] [review] what we need to get this signature fixed on beta? nsFontInflationData::FindFontInflationDataFor now is the #1 crash on 14.0b6 for native Fennec.
Comment 87 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-11 12:40:15 PDT
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #86)
> Is attachment 619739 [details] [diff] [review] what we need to get this
> signature fixed on beta? nsFontInflationData::FindFontInflationDataFor now
> is the #1 crash on 14.0b6 for native Fennec.

I don't know, but it might well help, and it's low risk.
Comment 88 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-06-11 12:44:13 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/3bd947c1514e
Comment 89 Jet Villegas (:jet) 2012-06-11 14:27:22 PDT
Please mark this closed/fixed when attachment 619739 [details] [diff] [review] lands on m-c. Any further crash investigation/repair will be tracked in bug 763702.
Comment 90 Brad Lassey [:blassey] (use needinfo?) 2012-06-11 16:50:59 PDT
Resolving this as fixed since the crash rate has plummeted on trunk. Let's open a new bug for any follow up work.
Comment 91 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-12 08:58:29 PDT
removing QAwanted from this bug as this particular portion of the crash bug seems fixed.  We need to figure out how to get reproducible steps in getting this crash on other portions of this crash signature.

Note You need to log in before you can comment on or make changes to this bug.