Firefox 3.0.9 has exposed a new topcrash @nsTextFrame::ClearTextRun(). One instance is bug 489322, involving the HTML Validator addon. We'll see soon if that's the only case because the author of that addon is about to release an update with a workaround (disables the crashing feature). Filing separate to track as a security bug. Crashes: http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.0.9&query_search=signature&query_type=exact&query=&date=&range_value=1&range_unit=weeks&do_query=1&signature=nsTextFrame%3A%3AClearTextRun() This appears to be exploitable given random addresses at the top of the stack bp-e5e76111-98f2-4785-9fe6-ba0582090421 bp-49a91d2b-b49c-4316-957e-d2c9b2090421 bp-87a98e87-4982-488f-8c11-6a2c72090421 bp-043a79b6-250a-4d52-8862-ef1d72090421 etc. Here's one with a comment that does NOT mention view source -- this one says they did a ctrl-f (find) and clicked the "Highlight All" button. bp-d31d7a90-09b4-4cf5-9baf-7b1952090422

Regression range is between the 2009-02-26 and 2009-02-27 builds on Windows only (looks like the extension has specific versions per-OS). http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2009-02-26&maxdate=2009-02-27&cvsroot=%2Fcvsroot Pretty sure this is either bug 431260 or bug 444027.

Given the build hour being 06, you probably want a range more like http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2009-02-26%2004:00&maxdate=2009-02-27%2007:00&cvsroot=%2Fcvsroot although you could reduce it by looking at tinderbox. In any case, I'd bet on bug 431260.

Changing OS field from Mac to Windows, since the crash report table shows 0 mac crashes, 1 linux crash, and 1113 windows crashes. (We can change the field to "All" after confirming a crash on another platform) Note: Bugzilla's auto-linking doesn't catch the "()" on the end of the first URL in comment 0 (the table of all matching crash reports) -- you have to copy and paste the full link (or click it and then manually add "()" at the end) in order for the link to work.

So we fixed 431260, which wasn't really a security problem, and we introduced this bug which probably is. Perhaps we need to be more picky about what we land on branch.

Daniel: Giving this over to you since you fixed bug 431260.

Since this isn't a problem on trunk or 1.9.1 (I presume), it's almost certain that a change in 1.9.1 fixed or prevented this bug. We just need to figure out which one. Bug 472776 is a strong candidate, Daniel, can you see if that patch fixes your Linux 1.9.0 opt build?

(In reply to comment #6) > Since this isn't a problem on trunk or 1.9.1 (I presume) That's correct -- I don't crash in my mozilla-central nightly, whereas I do in a 1.9.0.x nightly (per bug 489322 comment 24). > Bug 472776 is a strong candidate, Daniel, can you see if that patch fixes your > Linux 1.9.0 opt build? Yeah, I'll give that a shot.

I guess a surefire way to track this would be to take the patch in bug 431260 and bisect along the 1.9.1 branch between the 1.9.0 branch point and where the patch actually landed to identify where adding that patch to the 1.9.1 branch no longer causes a crash. Thanks Daniel!

It might not be bug 472776 that fixed it, since that landed on 1.9.1 on Feb 26, months after the patches in bug 431260 landed on 1.9.1.

Bug 467150 is another possibility.

So while waiting for my opt build to finish, I figured I'd test mozilla-central nightlies from right after the potential regressing patches landed, in the hopes that we crashed there too (and then we'd be able to do a binary-search for the fix range). I tried these mozilla-central nightly builds, to catch the just-landed fixes for bug 431341, bug 431260 / bug 455826, bug 444027 respectively: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1a2pre) Gecko/2008073002 Minefield/3.1a2pre Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081128 Minefield/3.1b3pre Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090211 Minefield/3.2a1pre However, none of them crashed. So unless I'm making a mistake, it looks like we never actually hit this crash on mozilla-central. :-/

(In reply to comment #10) > Bug 467150 is another possibility. That bug's patch indeed fixes this (at least, it fixes bug 489322), in my 190x opt build! I'm attaching a backported version of it here. It looks like 190x already includes the *first* patch on that bug (labeled "patch that landed on trunk"), whereas mozilla-central has the *second* patch (labeled "revised patch"). The two patches conflict, so this backport simply undoes the first patch and applies the second patch, making 1.9.0.x match mozilla-central.

(In reply to comment #12) > It looks like 190x already includes the *first* patch on that bug (labeled > "patch that landed on trunk") So, it turns out that patch was already applied on trunk because it was actually originally *part of* bug 455826's patch, attachment 348661 [details] [diff] [review], as posted on bugzilla (which I landed in 1.9.0.9 to fix bug 431260). However, when bug 455826 originally landed on mozilla-central, it was missing that exact chunk due to a minor oversight, as explained in bug 455826 comment 16. That comment explained that this chunk would be handled & land in a separate bug -- bug 467150. And *that* bug is where this chunk was landed, backed out, and then *improved* to form the patch that ended up actually fixing **this** bug here. Hope that makes sense. My bad for not seeing that bug 455826's patch got split up and improved elsewhere -- sorry about that! I think this all makes sense now, though -- we just need to "complete" the patch for bug 455826 by updating the chunk that was improved in bug 467150.

Comment on attachment 374220 [details] [diff] [review] 'revised patch' from bug 467150, backported to CVS trunk (In reply to comment #13) > I think this all makes sense > now, though -- we just need to "complete" the patch for bug 455826 by updating > the chunk that was improved in bug 467150. (That is to say -- we just need to land "patch for bug 467150, backported to CVS trunk", a.k.a. attachment 374220 [details] [diff] [review])

Comment on attachment 374220 [details] [diff] [review] 'revised patch' from bug 467150, backported to CVS trunk roc, just to confirm -- as the author of the patch on bug 467150, do you think it's suitable/safe for 1.9.0.x branch? FWIW, it doesn't seem to have caused any known regressions -- the only dependency on it (besides this bug here and the wikipedia line-wrap bug, both of which I've just added as dependencies) is bug 430332, and that relationship is only there because bug 467150 was part of the fix for bug 430332 (per bug 430332 comment 39).

This is a testcase that seems to reproduce this crash in current 1.9.0.x build: http://crash-stats.mozilla.com/report/index/6ba4cbb4-9c50-454a-a54a-747f32090423?p=1

Comment on attachment 374220 [details] [diff] [review] 'revised patch' from bug 467150, backported to CVS trunk It's my fault for creating such a messy bug structure in the first place.

I've confirmed that the attached patch (attachment 374220 [details] [diff] [review]) fixes the crash on mw22's testcase (attachment 374249 [details]), in my optimized build. Testcase crashes immediately before patching & doesn't crash after patching.

I can also confirm the crash with "Hightlight All" mentioned in comment 0 -- the crash report says to load http://msdn.microsoft.com/en-us/library/system.collections.hashtable(VS.71).aspx , hit Ctrl-F & type "using", and click "highlight all" --> crash) The patch fixes that crash as well.

Comment on attachment 374220 [details] [diff] [review] 'revised patch' from bug 467150, backported to CVS trunk Approved for 1.9.0.10, a=dveditz for release-drivers

Patch landed: Checking in layout/generic/nsTextFrameThebes.cpp; /cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v <-- nsTextFrameThebes.cpp new revision: 3.189; previous revision: 3.188 done

Comment on attachment 374220 [details] [diff] [review] 'revised patch' from bug 467150, backported to CVS trunk Approved for 1.9.0.10 (the real one!). a=ss

To be clear, my approval is for the GECKO190_20090406_RELBRANCH.

Comment on attachment 374220 [details] [diff] [review] 'revised patch' from bug 467150, backported to CVS trunk Landed on the release branch (GECKO190_20090406_RELBRANCH) Checking in generic/nsTextFrameThebes.cpp; /cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v <-- nsTextFrameThebes.cpp new revision: 3.188.2.1; previous revision: 3.188 done

Setting in-testsuite? flag, as a reminder to check in mw22's testcase once the fix has made it out to users.

(In reply to comment #16) > Created an attachment (id=374249) [details] > testcase > > This is a testcase that seems to reproduce this crash in current 1.9.0.x build: > http://crash-stats.mozilla.com/report/index/6ba4cbb4-9c50-454a-a54a-747f32090423?p=1 Verified the crash in 1.9.0.9 and that it no longer occurs in 1.9.0.10 on Linux with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.10) Gecko/2009042315 Firefox/3.0.10. I'll mark this verified1.9.0.10 after I can check on Windows.

Verified on Windows XP: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.10) Gecko/2009042316 Firefox/3.0.10 (.NET CLR 3.5.30729)

the code does not exist on 1.8 and the testcase does not trigger a crash there either.

I just took Martijn's test and made it into a crashtest. I'll upload patches for 1.9.1 and 1.9.0 as well so that we'll know if we ever regress this.

Comment on attachment 376294 [details] [diff] [review] Patch to add crashtest to trunk Two things: * Since this test uses setTimeout, could you add "reftest-wait" magic, for correctness? (Otherwise, if we're super-speedy, I think we could successfully load the test and move on to the next testcase before the setTimeout fires. Or something like that) * Also, we should reduce the setTimeout wait-time to 0 (or a small positive value), as long as it still crashes 1.9.0.9 builds, which I think it should. (We should we can avoid introducing biggish wait-times into our test suite, if at all possible, because they add up.) Otherwise, looks good!

Comment on attachment 376294 [details] [diff] [review] Patch to add crashtest to trunk One more thing -- looks like the testcase has no newline at end of file (as noted in the raw patch), which is messy -- can you fix that, too? r=dholbert, after addressing that & comment 37

oh, sorry, I just noticed you have reftest-wait magic already... I don't know how I missed that. Disregard that part of my comments :)

(In reply to comment #40) > oh, sorry, I just noticed you have reftest-wait magic already... I don't know > how I missed that. Disregard that part of my comments :) I was about to tell you I already did :) I'll run some tests and see what I can get away with decreasing the setTimeout call to and fix that newline. Thanks for the quick review on this. I'll repost the final test and carry your review forward.

Verified for 1.9.0.11 with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.11pre) Gecko/2009051404 GranParadiso/3.0.11pre.

> (In reply to comment #40) > I'll run some tests and see what I can > get away with decreasing the setTimeout call to and fix that newline. Thanks > for the quick review on this. I'll repost the final test and carry your review > forward. Hi Clint! I think you might've forgotten about this -- looks like this crashtest never got reposted with those fixes (reduced timeout if possible, & add missing newline).

Comment on attachment 376297 [details] [diff] [review] Patch for 1.9.1 r=dholbert on branch crashtests, with the same fixes from comment 43 that are still needed for the trunk crashtest.

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8eec4f84d97 Crashtest.