Last Comment Bug 791601 - "Assertion failure: checkmTextrun ? !mTextRun : !Properties().Get(UninflatedTextRunProperty())" with font inflation, -moz-column
: "Assertion failure: checkmTextrun ? !mTextRun : !Properties().Get(UninflatedT...
Status: VERIFIED FIXED
[adv-track-main17+]
: assertion, crash, sec-critical, testcase
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla18
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 885444
Blocks: stirdom randomstyles CVE-2012-3964
  Show dependency treegraph
 
Reported: 2012-09-16 19:07 PDT by Jesse Ruderman
Modified: 2013-07-02 13:01 PDT (History)
11 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
verified
+
verified
unaffected
verified


Attachments
testcase (requires pref) (crashes Firefox) (293 bytes, application/xhtml+xml)
2012-09-16 19:07 PDT, Jesse Ruderman
no flags Details
stack (11.47 KB, text/plain)
2012-09-16 19:44 PDT, Jesse Ruderman
no flags Details
stack + frame tree (10.16 KB, text/html)
2012-09-17 11:49 PDT, Mats Palmgren (:mats)
no flags Details
fix (2.93 KB, patch)
2012-09-17 11:54 PDT, Mats Palmgren (:mats)
roc: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review
fix (wdiff) (2.08 KB, patch)
2012-09-17 11:55 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
before/after frame trees with the fix (5.62 KB, text/plain)
2012-09-17 11:55 PDT, Mats Palmgren (:mats)
no flags Details

Description Jesse Ruderman 2012-09-16 19:07:47 PDT
Created attachment 661662 [details]
testcase (requires pref) (crashes Firefox)

With:
  user_pref("font.size.inflation.emPerLine", 15);
Loading the testcase triggers:

Assertion failure: checkmTextrun ? !mTextRun : !Properties().Get(UninflatedTextRunProperty()), at /Users/jruderman/trees/mozilla-central/layout/generic/nsTextFrameThebes.cpp:4373

Security-sensitive because the assertion was added in a security-sensitive bug.
Comment 1 Jesse Ruderman 2012-09-16 19:44:01 PDT
Created attachment 661664 [details]
stack
Comment 2 Mats Palmgren (:mats) 2012-09-17 11:49:07 PDT
Created attachment 661859 [details]
stack + frame tree

RemoveInFlows is trying to destroy the text frame marked red.
http://hg.mozilla.org/mozilla-central/annotate/3f0587ce1774/layout/generic/nsTextFrameThebes.cpp#l7273

This is pretty much the same situation as in bug 756241,
but with a block parent.  On line 7300, we explicitly excluded
this case...  I don't recall why we did that - maybe we thought
that FRAMES_ARE_EMPTY would prevent the same crash in blocks?

roc, do you recall?  do you see any reason why doing that block
unconditionally won't work?  (it fixes the crash locally)
Comment 3 Mats Palmgren (:mats) 2012-09-17 11:54:33 PDT
Created attachment 661861 [details] [diff] [review]
fix
Comment 4 Mats Palmgren (:mats) 2012-09-17 11:55:03 PDT
Created attachment 661862 [details] [diff] [review]
fix (wdiff)
Comment 5 Mats Palmgren (:mats) 2012-09-17 11:55:40 PDT
Created attachment 661863 [details]
before/after frame trees with the fix
Comment 6 Mats Palmgren (:mats) 2012-09-17 12:05:11 PDT
https://tbpl.mozilla.org/?tree=Try&rev=9f066ebad681
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-17 14:41:28 PDT
Comment on attachment 661861 [details] [diff] [review]
fix

Review of attachment 661861 [details] [diff] [review]:
-----------------------------------------------------------------

I don't know why we excluded the parent block here.

Clearing text runs should be safe in any case.
Comment 9 :Ehsan Akhgari (out sick) 2012-09-18 05:34:37 PDT
http://hg.mozilla.org/mozilla-central/rev/1d8ff0faf0fa
Comment 10 Robert Kaiser (not working on stability any more) 2012-10-17 15:01:57 PDT
This is marked sec-critical and tracking-firefox17+, are we planning to land this on beta?
Comment 11 Mats Palmgren (:mats) 2012-10-17 15:33:38 PDT
Comment on attachment 661861 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: sec-critical crash
Testing completed (on m-c, etc.): on trunk/aurora since 2012-09-18
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-18 10:41:44 PDT
Please also mark sec-approval? on this patch since it's sec-critical and looking for uplift we'll want to get that feedback as well to be able to time landing.
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-19 15:11:20 PDT
Comment on attachment 661861 [details] [diff] [review]
fix

Again, my bad, this doesn't need sec-approval please go ahead with uplift.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-10-22 20:55:21 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/7f450abfd318
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-12 15:29:57 PST
Kamil, can you please make an attempt at verifying this is fixed? It should be fixed in Firefox 17, 17esr, and 18beta.
Comment 16 Kamil Jozwiak [:kjozwiak] 2012-12-16 20:35:57 PST
When going through the following issue using the test case HTML file attached above, I receive the following crash every single time:

###!!! ASSERTION: math on NS_UNCONSTRAINEDSIZE: 'NS_UNCONSTRAINEDSIZE != aState.mReflowState.mComputedBorderPadding.left && NS_UNCONSTRAINEDSIZE != aState.mReflowState.ComputedWidth()', file e:/builds/moz2_slave/m-rel-w32-dbg/build/layout/generic/nsBlockFrame.cpp, line 1629

The error doesn't match the one that has been posted above, but it's making it difficult to test as firefox crashes whenever the .HTML file is opened. Is this the same issue as above just a different error message? Or is there something I am doing incorrectly?

I also set the following user preference as mentioned above: ("font.size.inflation.emPerLine", 15)

Received the above assertion crash when going through the following builds:

Firefox 16: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/10/2012-10-11-mozilla-release-debug/
Firefox 17: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/11/2012-11-20-mozilla-release-debug/
Firefox 18: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-12-16-mozilla-beta-debug/
Comment 17 Jesse Ruderman 2012-12-18 05:52:02 PST
(In reply to Kamil Jozwiak [:kjozwiak] from comment #16)
> ###!!! ASSERTION: math on NS_UNCONSTRAINEDSIZE

That's a non-fatal assertion, so you shouldn't experience it as a crash.  If you're on Windows, you might need to run with XPCOM_DEBUG_BREAK=warn.

(I tend to ignore any assertion that mentions NS_UNCONSTRAINEDSIZE, due to bug 575011.)
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-18 11:10:40 PST
Kamil, can you please retest with XPCOM_DEBUG_BREAK=warn as Jesse suggests?

Jesse, is this an environment variable that needs to be set or does this need to be set elsewhere?
Comment 20 Kamil Jozwiak [:kjozwiak] 2012-12-18 15:00:13 PST
Following prerequisites used before testing builds:
- Environment Variable: XPCOM_DEBUG_BREAK=warn
- about:config: ("font.size.inflation.emPerLine", 15)

Firefox 16 (Issue Reproduced): http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/10/2012-10-11-mozilla-release-debug/
Received Error: Assertion failure: checkmTextrun ? !mTextRun : !Properties().Get(UninflatedTextRunProperty()), at e:/builds/moz2_slave/m-rel-w32-dbg/build/layout/generic/nsTextFrameThebes.cpp:4248

Once the issue was reproduced several times with Firefox 16, used the following builds to ensure that the issue has been resolved:

Firefox 17 (Resolved): http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/11/2012-11-20-mozilla-release-debug/
- Ran through Firefox 17 and didn't receive the assertion crash that was reproduced with Firefox 16. Ensured that the above assertion failure didn't appear in the log file

Firefox 17esrpre BETA (Resolved): http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/11/2012-11-20-mozilla-esr17-debug/
- Ran through Firefox 17esrpre BETA and didn't receive the assertion crash that was reproduced with Firefox 16. Ensured that the above assertion failure didn't appear in the log file

Firefox 18 BETA (Resolved): http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012-12-16-mozilla-beta-debug/
- Ran through Firefox 18 BETA and didn't receive the assertion crash that was reproduced with Firefox 16. Ensured that the above assertion failure didn't appear in the log file
Comment 21 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-18 15:11:17 PST
Thank you, Kamil. I think this is sufficient to call this verified fixed. Jesse (or anyone else) feel free to revert my flag changes if there's something else we can do.
Comment 22 Mats Palmgren (:mats) 2013-05-13 07:31:51 PDT
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfaa2e423b1f
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-05-13 13:44:44 PDT
https://hg.mozilla.org/mozilla-central/rev/cfaa2e423b1f

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