Last Comment Bug 373756 - [FIX]crash at [@ nsCSSFrameConstructor::ConstructFrame] with "Web Developer Toolbar" and custom XPI
: [FIX]crash at [@ nsCSSFrameConstructor::ConstructFrame] with "Web Developer T...
Status: RESOLVED FIXED
regression from 267833?
: crash, regression, verified1.8.1.8
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P1 critical with 1 vote (vote)
: mozilla1.9alpha5
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 267833 373727 373944
  Show dependency treegraph
 
Reported: 2007-03-13 05:14 PDT by Henrik Gemal
Modified: 2011-06-13 10:01 PDT (History)
11 users (show)
dbaron: blocking1.9+
dveditz: blocking1.8.1.8+
dveditz: blocking1.8.0.14-
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Webdeveloper XPI (173.64 KB, application/x-xpinstall)
2007-03-14 06:23 PDT, Henrik Gemal
no flags Details
Inhouse XPI (25.63 KB, application/x-xpinstall)
2007-03-14 06:24 PDT, Henrik Gemal
no flags Details
minimal XPI to reproduce crash (install on top of webdeveloper) (1.55 KB, application/x-xpinstall)
2007-03-14 06:53 PDT, Henrik Gemal
no flags Details
call stack for the removeChild call in the <toolbar> constructor (8.78 KB, text/plain)
2007-03-14 10:43 PDT, Nickolay_Ponomarev
no flags Details
simpler XPI (1.01 KB, application/x-xpinstall)
2007-03-16 01:10 PDT, Nickolay_Ponomarev
no flags Details
Proposed patch (4.75 KB, patch)
2007-04-16 18:13 PDT, Boris Zbarsky [:bz]
jonas: review+
jonas: superreview+
dveditz: approval1.8.1.8+
Details | Diff | Splinter Review

Description Henrik Gemal 2007-03-13 05:14:56 PDT
TB30191020M, TB30190957Y, TB30190939W, TB30186402G

Seems to be related to the Web Developer Toolbar. If I disable that Firefox
starts fine
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070312
Minefield/3.0a3pre ID:2007031204 [cairo]
Comment 1 Nickolay_Ponomarev 2007-03-13 07:16:11 PDT
Note bz's comment in bug 360992: "with detailed steps to reproduce starting from a vanilla profile (including URIs to the exact XPIs you installed or detailed click sequences to allow someone else with no experience installing extensions to install identical ones)."

Please spend time to ensure this crash can be reproduced by a developer.

Did this regress recently for you? Did you crash more than 30 times to make this show up at http://talkback-public.mozilla.org/reports/firefox/ or is someone else crashing too?
Comment 2 Henrik Gemal 2007-03-14 06:23:47 PDT
Created attachment 258534 [details]
Webdeveloper XPI
Comment 3 Henrik Gemal 2007-03-14 06:24:52 PDT
Created attachment 258535 [details]
Inhouse XPI

To reproduce:
- install the two XPI attached to the bug and restart. Crash!
Comment 4 Henrik Gemal 2007-03-14 06:47:43 PDT
I narrowed the main.xul file down to:

<?xml version="1.0"?>
<overlay xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
<toolbox id="navigator-toolbox">
<toolbar>
<textbox />
</toolbar>
</toolbox>
</overlay>

The bug has something to do with
<textbox />

We're using this XPI in our company so having this crash is not good :(
Comment 5 Henrik Gemal 2007-03-14 06:53:45 PDT
Created attachment 258538 [details]
minimal XPI to reproduce crash (install on top of webdeveloper)

I've narrowed the XPI down to the bare minimal
Comment 6 Martijn Wargers [:mwargers] (not working for Mozilla) 2007-03-14 07:18:45 PDT
Thanks for minimizing, Henrik. I don't crash after installing "minimal XPI to reproduce crash". Do I need something else to install also, to see the crash?
Comment 7 Henrik Gemal 2007-03-14 07:20:05 PDT
did you install the web developer toolbar?
what build are you running?

Mine is:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070313 Minefield/3.0a3pre ID:2007031304 [cairo]
Comment 8 Nickolay_Ponomarev 2007-03-14 07:23:56 PDT
I am able to reproduce it and will try to look into it, hopefully today.
Comment 9 Nickolay_Ponomarev 2007-03-14 10:42:07 PDT
The crash happens because this loop assumes the child list of aContent does not change (as far as I can see):
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1331#11255
...and in this case the child list does change. Here's what happens:
* nsCSSFrameConstructor::ProcessChildren processes the main <toolbox>'s children, which look like this:
<XUL toolbox@031323A8 intrinsicstate=[00000000] refcount=9<
  <XUL toolbar@031324C0 currentset="menubar-items,spring,throbber-box" intrinsicstate=[00000000] refcount=6<
  >
  <XUL toolbarpalette@03AE1980 intrinsicstate=[00000000] refcount=3<
  >
  <XUL toolbar@0313A398 currentset="back-button,forward-button,reload-button,stop-button,home-button,urlbar-container,search-container" intrinsicstate=[00000000] refcount=2<
  >
  <XUL toolbarset@0313B0B8 intrinsicstate=[00000000] refcount=2<>
  <XUL toolbar@0313B360 currentset="personal-bookmarks" intrinsicstate=[00000000] refcount=2<>
  <XUL toolbar@03B580F0 intrinsicstate=[00000000] refcount=1<
  >
  <XUL toolbar@03E00220 intrinsicstate=[00000000] refcount=2<>
  <XUL toolbar@03E004C8 intrinsicstate=[00000000] refcount=2<
  >
  <XUL toolbar@03DC9EB8 intrinsicstate=[00000000] refcount=2<
  >
  <XUL toolbar@03DCA680 intrinsicstate=[00000000] refcount=2<
  >
  <XUL toolbar@03DED3A0 intrinsicstate=[00000000] refcount=2<
  >
  <XUL toolbar@03DEE4A0 intrinsicstate=[00000000] refcount=2<
  >
>
(deeper children skipped)
* While processing the toolbar@03B580F0, which is the one coming from the overlay from the "minimal XPI to reproduce crash", a <textbox> is created. During its initialization somehow nsDocument::EndUpdate happens, which a <toolbar> to be attached (synchronously).
* The <toolbar> binding is evil enough to remove the <toolbarpalette> from the DOM in its constructor:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/content/widgets/toolbar.xml&rev=1.33#118

For the reference, the stack for the crash is:
nsCSSFrameConstructor::ConstructFrame(aState={...}, aContent=0x00000000, aParentFrame=0x0474e1dc, aFrameItems={...})  Line 7411
nsCSSFrameConstructor::ProcessChildren(aState={...}, aContent=0x0435dc80, aFrame=0x0474e1dc, aCanHaveGeneratedContent=0x00000000, aFrameItems={...}, aParentIsBlock=0x00000000)  Line 11261
nsCSSFrameConstructor::ConstructXULFrame(aState={...}, aContent=0x0435dc80, aParentFrame=0x03d7f024, aTag=0x03cc4cf0, aNameSpaceID=0x00000009, aStyleContext=0x0474df8c, aFrameItems={...}, aXBLBaseTag=0x00000000, aHasPseudoParent=0x00000000, aHaltProcessing=0x0012eb8c)  Line 6143
nsCSSFrameConstructor::ConstructFrameInternal(aState={...}, aContent=0x0435dc80, aParentFrame=0x03d7f024, aTag=0x03cc4cf0, aNameSpaceID=0x00000009, aStyleContext=0x046fe4b4, aFrameItems={...}, aXBLBaseTag=0x00000000)  Line 7566
nsCSSFrameConstructor::ConstructFrame(aState={...}, aContent=0x0435dc80, aParentFrame=0x03d7f024, aFrameItems={...})  Line 7428
nsCSSFrameConstructor::ProcessChildren(aState={...}, aContent=0x04204fb8, aFrame=0x03d7f024, aCanHaveGeneratedContent=0x00000001, aFrameItems={...}, aParentIsBlock=0x00000000)  Line 11261
nsCSSFrameConstructor::ConstructDocElementFrame(aState={...}, aDocElement=0x04204fb8, aParentFrame=0x03d7ebd0, aNewFrame=0x0012ef6c)  Line 4374
nsCSSFrameConstructor::ContentInserted(aContainer=0x00000000, aChild=0x04204fb8, aIndexInContainer=0x00000000, aFrameState=0x00000000, aInReinsertContent=0x00000000)  Line 8841
PresShell::InitialReflow(aWidth=0x00001af4, aHeight=0x00000000)  Line 2441
nsXULDocument::StartLayout()  Line 2030

I'm not familiar with how this used to (and how it is supposed to) work, so not sure what the proper fix is.

I think I'll blame the fix for bug 267833, especially since this works in 2007030804.
Comment 10 Nickolay_Ponomarev 2007-03-14 10:43:44 PDT
Created attachment 258564 [details]
call stack for the removeChild call in the <toolbar> constructor
Comment 11 Nickolay_Ponomarev 2007-03-14 10:44:56 PDT
Comment on attachment 258534 [details]
Webdeveloper XPI

(Note that you need to bump the maxversion in install.rdf to install on trunk :( )
Comment 12 Nickolay_Ponomarev 2007-03-14 10:50:14 PDT
(And I have no idea why webdeveloper is necessary to reproduce this.)
Comment 13 Nickolay_Ponomarev 2007-03-16 01:10:40 PDT
Created attachment 258760 [details]
simpler XPI

Actually the only role webdeveloper plays is making the <toolbar> with the <textbox> that causes brokenness not the last child of <toolbox>. Here's a simpler XPI that can be installed instead of the above ones to reproduce the crash.
Comment 14 Boris Zbarsky [:bz] 2007-04-16 18:13:45 PDT
Created attachment 261743 [details] [diff] [review]
Proposed patch

The idea here is similar to what we do with scripts: only process constructors synchronously in EndUpdate() if there weren't any pending ones at the beginning of the update.

The other option would be to only process in EndUpdate() if bindings got added to the queue during the update; this is not quite the same thing, unlike in the script case.

This patch does fix this bug, bug 373719 (with the relevant patches reverted), and bug 373727.

Nickolay, thank you very much for digging into this!  It made it pretty clear what needed to be done.
Comment 15 Boris Zbarsky [:bz] 2007-04-16 18:16:59 PDT
If someone understands the UI end enough to write a standalone (non-extension) test we could check in, that would rock...
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2007-05-30 16:47:08 PDT
Comment on attachment 261743 [details] [diff] [review]
Proposed patch

Processing even if bindings got added during the update cycle sounds wrong, I like the approach in this patch much better.
Comment 17 Boris Zbarsky [:bz] 2007-05-30 20:45:57 PDT
Jonas, thanks for checking in the fix!
Comment 18 Boris Zbarsky [:bz] 2007-07-09 18:38:47 PDT
Comment on attachment 261743 [details] [diff] [review]
Proposed patch

Need this to land bug 267833
Comment 19 Daniel Veditz [:dveditz] 2007-09-06 16:21:02 PDT
Comment on attachment 261743 [details] [diff] [review]
Proposed patch

approved for 1.8.1.7, a=dveditz for release-drivers
Comment 20 Boris Zbarsky [:bz] 2007-09-14 14:20:59 PDT
I've rolled the merged-to-branch version of this into the patch for bug 267833.
Comment 21 Daniel Veditz [:dveditz] 2007-10-01 17:49:13 PDT
the merged patch for bug 267833 landed. This is fixed too, right? Seems to wfm but I didn't test seriously enough to call this verified
Comment 22 Carsten Book [:Tomcat] 2007-10-16 15:12:36 PDT
verified fixed 1.8.1.8 using the steps to reproduce in this bug with the 2 xpi Files and Webdeveloper Toolbar on Mozilla/5.0 (Windows; U; Windows NT 5.2; de; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8 ID:2007100816

No crash -> adding verified keyword
Comment 23 Daniel Veditz [:dveditz] 2007-12-03 14:25:39 PST
We're not going to take the regression risk from the XBL re-write on the 1.5 product line.

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