Closed Bug 749186 Opened 12 years ago Closed 12 years ago

crash in nsFontInflationData::FindFontInflationDataFor at crash address 0x28 (((nsIFrame*)0)->GetStateBits())

Categories

(Core :: Layout, defect, P1)

14 Branch
ARM
Android
defect

Tracking

()

RESOLVED FIXED
mozilla16
Tracking Status
firefox14 + fixed
firefox15 + fixed
blocking-fennec1.0 --- betaN+

People

(Reporter: scoobidiver, Assigned: jwir3)

References

(Blocks 1 open bug, )

Details

(5 keywords, Whiteboard: [native-crash][readability])

Crash Data

Attachments

(6 files, 7 obsolete files)

3.57 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.74 MB, video/webm
Details
522 bytes, text/html
Details
2.36 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
4.92 KB, patch
Details | Diff | Splinter Review
10.09 KB, patch
jwir3
: review+
Details | Diff | Splinter Review
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
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → +
Whiteboard: [native-crash] → [native-crash][readability]
Assignee: nobody → sjohnson
Attached patch b749186 (obsolete) — Splinter Review
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. :(
Attachment #619637 - Flags: review?(dbaron)
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 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.
Attachment #619637 - Flags: review?(dbaron) → review-
Attached patch b749186 (v2) (obsolete) — Splinter Review
(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.
Attachment #619637 - Attachment is obsolete: true
Attachment #619716 - Flags: review?(dbaron)
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 "(".
Attachment #619716 - Flags: review?(dbaron) → review-
Attached patch b749186 (v3)Splinter Review
(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.
Attachment #619716 - Attachment is obsolete: true
Attachment #619739 - Flags: review?(dbaron)
Comment on attachment 619739 [details] [diff] [review]
b749186 (v3)

r=dbaron
Attachment #619739 - Flags: review?(dbaron) → review+
looks like comment 9 accidentally reset the flags, so back to tracking with them!
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeba341b7e45
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/eeba341b7e45
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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
Attachment #619739 - Flags: approval-mozilla-aurora?
(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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Attached video 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
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)
Awesome. I'll look into that tomorrow. Thanks for the STR.
Keywords: reproducible
(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...
Whoops. Set the flag accidentally. 

Also, what version of android do you have on your phone? Are you using ICS?
Shortened link:
http://bit.ly/IHL5V3
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
We are leaving all non-beta+ bugs nominated for Aurora approval in the queue until FN14 Beta 1 is signed off on by QA.
Attached file testcase (uses enhanced privileges) (obsolete) —
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
To reproduce with this testcase, install this extension:
http://people.mozilla.org/~mwargers/extensions/specialpowers.xpi
Keywords: mobile, testcase
Comment on attachment 619739 [details] [diff] [review]
b749186 (v3)

Renom for the next patch
Attachment #619739 - Flags: approval-mozilla-aurora?
Priority: -- → P1
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().
(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.
Attached patch b749186-followup1 (obsolete) — Splinter Review
Followup that sets the font inflation state bits on frames, whether or not font inflation is enabled at frame initialization.
Attachment #624552 - Flags: review?(dbaron)
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
(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.
(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.
Blocks: 756999
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.
I also crashed once with this stacktrace (or similar one) when the viewport of the device was changed (portrait to landscape), btw.
Attached patch bug749186-followup1 (obsolete) — Splinter Review
Patch that regenerates the frame tree when these preferences change.
Attachment #624552 - Attachment is obsolete: true
Attachment #626944 - Flags: review?(dbaron)
Mochitest that should trigger a crash for this bug.
Attachment #626946 - Flags: review?(dbaron)
This crash is currently Fennec 15.0a1's #3 topcrash for the past 3 days.
Attached patch bug749186-followup1 (v2) (obsolete) — Splinter Review
Slightly modified the code in the UpdateAfterPreferencesChanged handler to get font-inflation tests to pass without assertions.
Attachment #626944 - Attachment is obsolete: true
Attachment #626944 - Flags: review?(dbaron)
Attachment #627290 - Flags: review?(dbaron)
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
Attachment #627290 - Flags: review?(dbaron) → review+
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
Attachment #626946 - Flags: review?(dbaron) → review+
(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?
(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.
(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.
Just waiting for try results, then I will be pushing this to inbound -
https://tbpl.mozilla.org/?tree=Try&rev=266ff8a1e2b7
> 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?
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.)
(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.
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?
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?
(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.
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...
(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?
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?
Attachment #627290 - Attachment is obsolete: true
Attachment #629384 - Flags: feedback?(dbaron)
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.
Blocks: 747720
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
and in fact, it seems like this happens even after startup.
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...
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?)
(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...
(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.
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).
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.
blocking-fennec1.0: + → betaN+
Attached patch b749186-followup-cached (obsolete) — Splinter Review
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).
Attachment #630372 - Flags: review?(tnikkel)
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
Attachment #630372 - Flags: review?(tnikkel) → review+
[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.
Attachment #630372 - Attachment is obsolete: true
Attachment #630432 - Flags: review+
Attachment #630432 - Flags: approval-mozilla-beta?
Comment on attachment 630432 [details] [diff] [review]
b749186-followup-cached (v2)

[Triage Comment]
Attachment #630432 - Flags: approval-mozilla-beta?
Attachment #630432 - Flags: approval-mozilla-beta+
Attachment #630432 - Flags: approval-mozilla-aurora+
Had a bad merge, and was backed out on beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/23f3e1dfaa6c
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla15 → mozilla16
There are still crashes in 16.0a1/20120606 that contains the patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
I can no longer reproduce this with Special Powers API installed and attachment 622688 [details]. Can you post STR?
Attachment #622687 - Attachment is obsolete: true
(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.
(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
Please confirm if the STR still repros in the Beta build. If not, please find us a new URL to crash this.
Keywords: qawanted
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 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
Attachment #619739 - Flags: approval-mozilla-beta?
Summary: crash in nsFontInflationData::FindFontInflationDataFor → crash in nsFontInflationData::FindFontInflationDataFor at crash address 0x28 (((nsIFrame*)0)->GetStateBits())
(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.
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.
Attachment #619739 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Blocks: 763702
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.
Resolving this as fixed since the crash rate has plummeted on trunk. Let's open a new bug for any follow up work.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
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.
Keywords: qawanted
Blocks: 766599
Attachment #629384 - Flags: feedback?(dbaron)
You need to log in before you can comment on or make changes to this bug.