348688 - Crash/hang [@ nsLineBox::RemovePlaceholderDescendantsOf ][@ ntdll.dll] on avrev.com with Adblock Plus 0.7.1.1 enabled

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Weston Carloss]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060814 BonEcho/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060814 BonEcho/2.0b1

When I visit avrev.com with Adblock Plus enabled Bon Echo hangs or crashes.  Disabling Adblock Plus allows the site to work correctly.

Reproducible: Always

Steps to Reproduce:
1. Visit avrev.com with Adblock Plus enabled.
2. If Bon Echo does not immediately hang or crash, try clicking a link.





Using crash2 I have this Talkback ID:  TB22100260K

Comment 1

[Weston Carloss]

Here's a Talkback ID with Minefield crashing on the same site with Adblock Plus.

TB22101355E

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060814 Minefield/3.0a1 ID:2006081404 [cairo]

Comment 2

[Martijn Wargers (dead)]

Adblock Plus 0.7.1.1 can be downloaded from here:
https://addons.mozilla.org/firefox/1865/

I don't get a crash with 2006-07-28 branch build, I get the crash with 2006-07-30 branch build.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-28+05&maxdate=2006-07-30+10&cvsroot=%2Fcvsroot
I don't get a crash with 2006-07-28 1.5.0.x build, I get the crash with 2006-08-14 1.5.0.x build.
I don't get a crash at all with trunk builds.

I think the fix for bug 337883 caused this somehow. It's the only patch, that also was checked in the 1.8.0.x tree.

Getting a minimised testcase out of this would be very tough.

Comment 3

[Mats Palmgren (inactive)]

I can reproduce it in a Firefox trunk debug build on Linux. I'll have a look.
(Marking Security-Sensitive because it uses freed memory.)

Comment 4

[Martijn Wargers (dead)]

Well, this was widely discussed in http://forums.mozillazine.org/viewtopic.php?t=451790&start=45
and the bug report was mentioned in:
http://forums.mozillazine.org/viewtopic.php?t=451790&start=45#2432005

Comment 5

[Mats Palmgren (inactive)]

Attachment: Testcase #1

When we remove a float inside an inline we fail to update the float
containing block's (FCB) float cache which will contain the destroyed
placeholder frame. Removing another frame directly in the FCB will
crash in nsLineBox::RemovePlaceholderDescendantsOf.

Making nsInlineFrame::RemoveFrame() also update the float cache of
the nearest FCB fixes the crash for me... patch soon, need to investigate
if there are other cases and the performance impact first...

Comment 6

[Mats Palmgren (inactive)]

This is more or less the same as bug 337883. Both are in fact regressions
from bug 310638, this line to be precise:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1249&root=/cvsroot&mark=9775#9745

Nulling out mOutOfFlowFrame this early breaks the float cache which depends on
it to find out which FC entry is associated with the float:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsBlockFrame.cpp&rev=3.786&root=/cvsroot&mark=5564,5575#5557
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsLineBox.cpp&rev=1.112&root=/cvsroot&mark=513,517#507
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsLineBox.cpp&rev=1.112&root=/cvsroot&mark=913#907

I think the correct fix is to remove the SetOutOfFlowFrame(nsnull) line
and backout bug 337883. I will run some more tests on this...

Comment 7

[Daniel Veditz [:dveditz]]

1.8.0.7 code freeze is coming up soon (Aug 26 I think). Is it reasonable to expect a patch by then?

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The real fix for this bug and bug 337883 is to remove that
SetOutOfFlowFrame(nsnull) line (the rest of the patch just backs out
bug 337883). I wasn't aware that the float cache depends on that pointer
being intact during frame destruction, mea culpa.

I have tested this patch on 1.8/1.8.0 branches as well.

Comment 9

[Mats Palmgren (inactive)]

I'm a bit surprised that the various testing tools we have does not trigger
this crash. The testcase in this bug for example is fairly simple and looks
to me as if should be triggered, especially if you waste many, many CPU-hours
on it ;-)

Comment 10

[Martijn Wargers (dead)]

Bug 339651 seems related.

Comment 11

[Mats Palmgren (inactive)]

Thanks Martijn. The patch fixes bug 339651 too. Further analysis on that bug.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 234014 [details] [diff] [review]
Patch rev. 1

>Index: layout/base/nsCSSFrameConstructor.cpp
>-        ((nsPlaceholderFrame*)childFrame)->SetOutOfFlowFrame(nsnull);

Add a comment here that we should not SetOutOfFlowFrame(nsnull), possibly with a pointer to either this bug or the float cache code or both?

r=bzbarsky with that.

Comment 13

[Mats Palmgren (inactive)]

Attachment: rev. 1 + a comment

Comment 14

[Mats Palmgren (inactive)]

Checked in to trunk at 2006-08-17 06:15 PDT.
(Will ask for branch approvals in a few days...)

-> FIXED

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 234014 [details] [diff] [review]
Patch rev. 1

approved for 1.8.0 branch, a=dveditz for drivers

Comment 16

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_0_BRANCH at 2006-08-21 13:34 PDT.

Comment 17

[Mike Schroepfer]

Comment on attachment 234014 [details] [diff] [review]
Patch rev. 1

a=schrep for drivers - approving all [181approval pending] bugs now that tree is open.

Comment 18

[Weston Carloss]

Has this for certain been checked in on the Branch for 2.0?  I no longer see crashes on the trunk, but I believe I just got the same crash on the same site with Adblock Plus 0.7.1.2.

TB22441403X

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060823 BonEcho/2.0b2 ID:2006082303

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Has this for certain been checked in on the Branch for 2.0? 

It has for certain NOT been checked in.  When it is, the "fixed1.8.1" keyword will be added.

Comment 20

[Mats Palmgren (inactive)]

Checked in to MOZILLA_1_8_BRANCH at 2006-08-23 21:16 PDT.

Comment 21

[Jay Patel [:jay]]

v.fixed on both branches with 8/24 nightly builds, no crashes with AdBlock Plus and testcase in this bug.

Weston:  Are you still crashing with today's (20060824) Bon Echo builds?

Comment 22

[Weston Carloss]

Neither avrev.com or the testcase crash with today's build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060824 BonEcho/2.0b2 ID:2006082403).  This has been fixed.

Comment 23

[Jesse Ruderman]

I checked in Mats Palmgren's testcase as a crashtest.