Bugzilla@Mozilla – Bug 476547
Crash [@ nsTextFrameUtils::TransformText] with MathML, lquote, CSS quotes property
Last modified: 2010-10-30 11:16:59 PDT
Summon comment box
Created attachment 360186 [details] testcase 1 (assertions only) Testcase 1: ###!!! ASSERTION: negative length: 'GetContentEnd() - mContentOffset >= 0', file nsTextFrame.h, line 307 ###!!! ASSERTION: bad index: 'PRUint32(aIndex) < mState.mLength', file nsTextFragment.h, line 184 ###!!! ASSERTION: integer overflow: 'mMaxTextLength <= mMaxTextLength + aFrame->GetContentLength()', file layout/generic/nsTextFrameThebes.cpp, line 1222 Testcase 2: Null deref crash [@ nsTextFrameUtils::TransformText]
Created attachment 360187 [details] testcase 2 (crashes Firefox when loaded)
It would be nice if this were fixed, so I could look for textframe assertions again. I have to ignore a bunch of them because of this known bug.
The null deref in testcase 2 is now covered by bug 515771, this bug can be about the assertions. Unless this bug is saying the assertions are incorrect, "integer overflow" sounds like a security bug that needs hiding.
The bug here is that MathML's quote-setting is rather naive: /////////////////////////////////////////////////////////////////////////// // For <ms>, it is assumed that the mathml.css file contains two rules: // ms:before { content: open-quote; } // ms:after { content: close-quote; } // With these two rules, the frame construction code will // create inline frames that contain text frames which themselves // contain the text content of the quotes. // So the main idea in this code is to see if there are lquote and // rquote attributes. If these are there, we ovewrite the default // quotes in the text frames. However, due to RTL and line-breaking the anonymous content for :before and :after can actually comprise multiple frames each, which is what's happening here.
Created attachment 404767 [details] [review] fix This fixes it. It turns out that because of changes to the frame tree since this was first implemented, rquote doesn't work at all on trunk. So I'm adding a test that lquote and rquote actually work.
BTW lquote and rquote should really be implemented by mapping into a style rule somehow, but I don't want to invest in that right now.
http://hg.mozilla.org/mozilla-central/rev/47aeba980aeb
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/44db41f43111
also crashes 1.9.1 bp-4d08d7b7-3308-4cce-a33d-433672100428 and 1.9.0 bp-885d6b09-9553-4f8c-8d07-ee5d82100428
Need a mozilla-1.9.1 branch patch here, please.
Created attachment 442814 [details] [review] 1.9.1 patch 1.9.2 patch applies fine after fixing file names
Comment on attachment 442814 [details] [review] 1.9.1 patch Approved for 1.9.1.11, a=dveditz for release-drivers
This caused reftest failures on 1.9.1 on all platforms. See: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1277096000.1277096773.19239.gz (Mac) http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1277096175.1277097521.21730.gz (Linux) http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1277095999.1277096721.19141.gz (Windows) SeaMonkey2.0 shows the exact same reftest failures, so it's surely not a one-off.
REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/mozilla-1.9.1-macosx-unittest-reftest/build/reftest/tests/layout/reftests/mathml/quotes-1.xhtml | timed out waiting for reftest-wait to be removed (after onload fired)
I backed it out to fix the test failure. I don't have time to work on this now.
Can we bump this to the next release? I don't have time to work out why the test failed on branch. It's probably something simple though.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d7bf87af553a
So, I think I shouldn't have landed this at all, since it has caused the same orange as comment 13. I backed it out: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/801b22550e2e I think roc meant to switch the blocking flag to ? in comment 16.
Comment on attachment 442814 [details] [review] 1.9.1 patch Nominating for the next release.
Comment on attachment 442814 [details] [review] 1.9.1 patch I'm going to minus this patch for 1.9.1.11 because we'll need some sort of test fix. If you create a merged patch we can approve that, if it's just an additive test patch we can approve both. We'll wait for the explicit .12 requests to know which way to go.
I still don't have time to work on this.
Moving 1.9.1 blocking flag forward then.
Created attachment 478068 [details] [review] 1.9.1 patch that works OK, the problem was simply that reftests on 1.9.1 don't support the MozReftestInvalidate event, so the test doesn't run properly. Fixed that by doing a setTimeout after onload instead. It won't necessarily test dynamic updates reliably, but that doesn't matter. This is ready to land.
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f6d450b3aff7
There's missing NULL check for quoteContent which was in the original code: - if (quoteContent && quoteContent->IsNodeOfType(nsINode::eTEXT)) { [...] + if (!quoteContent->IsNodeOfType(nsINode::eTEXT)) Is it an intended change?
It's fine, quoteContent can't be null.