Last Comment Bug 344300 - Crash [@ nsFrameItems::AddChild ] on 1.8.0.5 and 1.8.1 branch
: Crash [@ nsFrameItems::AddChild ] on 1.8.0.5 and 1.8.1 branch
Status: RESOLVED FIXED
[sg:nse null-deref]
: crash, testcase, verified1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks: 344056
  Show dependency treegraph
 
Reported: 2006-07-11 15:32 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.74 KB, application/xhtml+xml)
2006-07-11 15:32 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
testcase2 (111 bytes, text/html)
2006-07-27 08:22 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
Patch rev. 1 (1.51 KB, patch)
2007-05-05 00:17 PDT, Mats Palmgren (:mats)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2006-07-11 15:32:08 PDT
See upcoming testcase, which crashes current branches, it doesn't crash current trunk build.
If desired, I can reduce the testcase.

Talkback ID: TB20840965W
nsFrameItems::AddChild  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 744]
nsCSSFrameConstructor::ConstructFrameByDisplayType  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 6891]
nsCSSFrameConstructor::CantRenderReplacedElement  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp, line 11028]
CantRenderReplacedElementEvent::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp, line 4090]
0x778b0c24
0x00200064
0x0c718d08
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-07-11 15:32:40 PDT
Created attachment 228867 [details]
testcase
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-07-11 15:37:10 PDT
Ctrl++ to zoom the testcase seems to trigger the crash bug 343206 again on trunk.
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2006-07-27 08:22:17 PDT
Created attachment 230900 [details]
testcase2

A different testcase that also triggers this same crash in branch builds.
Comment 4 Daniel Veditz [:dveditz] 2007-04-02 18:07:09 PDT
This looks like a safe null-deref on the branch and doesn't happen on the trunk. Any objection to removing the security-sensitive flag on this bug?
Comment 5 Daniel Veditz [:dveditz] 2007-04-02 18:11:26 PDT
I don't see the trunk crash described in comment 2 anymore.
Comment 6 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-04-03 03:08:06 PDT
(In reply to comment #4)
> This looks like a safe null-deref on the branch and doesn't happen on the
> trunk. Any objection to removing the security-sensitive flag on this bug?

No, I don't have any objection to that (if you want to remove the security-sensitive flag, then I have no objection, in general).
Comment 7 Mats Palmgren (:mats) 2007-05-05 00:14:12 PDT
CantRenderReplacedElement() calls ConstructFrameByDisplayType() which only
handles a limited set of cases since it's normally called at the end of
ConstructFrameInternal() (after XUL frame creation an such).
We have <object style="display: -moz-grid-line;"> which isn't handled
so we fall into the UNREACHED at the end, since we haven't created
a frame in this case we will call AddChild with a null frame.
This is a 100% null-deref, so not a security problem.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=MOZILLA_1_8_BRANCH&root=/cvsroot&mark=6893,6905#6842
Comment 8 Mats Palmgren (:mats) 2007-05-05 00:17:41 PDT
Created attachment 263844 [details] [diff] [review]
Patch rev. 1
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-05-05 02:55:24 PDT
So maybe this should not be fixed on branch then?
I mean, iirc, I hit crash a lot on branch. It might be that after this fix, I could hit something worse (not that I test that much on branch, unfortunately).
Comment 10 Mats Palmgren (:mats) 2007-05-05 05:13:07 PDT
> It might be that after this fix, I could hit something worse

I don't think so.  The patch results in the content in this case will have
no frame at all, which is a common and well tested state.  I think the
risk of this patch enabling new and untested code paths is low.

OTOH, it's highly unlikely that any real web pages will trigger this crash
by accident...
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-05-06 11:57:52 PDT
Comment on attachment 263844 [details] [diff] [review]
Patch rev. 1

Sure, I guess.  I hate the branch.  ;)
Comment 13 Daniel Veditz [:dveditz] 2007-06-25 11:11:01 PDT
Comment on attachment 263844 [details] [diff] [review]
Patch rev. 1

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers
Comment 14 Mats Palmgren (:mats) 2007-07-01 19:40:18 PDT
Checked in to MOZILLA_1_8_BRANCH:
layout/base/nsCSSFrameConstructor.cpp:  1.1110.6.77

Checked in to MOZILLA_1_8_0_BRANCH:
layout/base/nsCSSFrameConstructor.cpp:  1.1110.6.12.2.56
Comment 15 Carsten Book [:Tomcat] 2007-07-05 05:46:59 PDT
verified fixed on 1.8.1.5 using Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.5pre) Gecko/20070705 BonEcho/2.0.0.5pre ID:2007070504 and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.5pre) Gecko/2007070503 BonEcho/2.0.0.5pre on Fedora F7 - no crash on testcases - adding verified keyword
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2007-08-21 11:03:40 PDT
This is verified fixed on 1.8.0.13
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2007-08-21 11:06:28 PDT
(In reply to comment #16)
> This is verified fixed on 1.8.0.13
> 

Sorry, this was on Linux.  I should have specified this.
Comment 18 Bob Clary [:bc:] 2009-04-24 11:03:43 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/b480ed41fe22

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