373756 - [FIX]crash at [@ nsCSSFrameConstructor::ConstructFrame] with "Web Developer Toolbar" and custom XPI

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Henrik Gemal]

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]

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]

Attachment: Webdeveloper XPI

Comment 3

[Henrik Gemal]

Attachment: Inhouse XPI

To reproduce:
- install the two XPI attached to the bug and restart. Crash!

Comment 4

[Henrik Gemal]

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]

Attachment: minimal XPI to reproduce crash (install on top of webdeveloper)

I've narrowed the XPI down to the bare minimal

Comment 6

[Martijn Wargers (dead)]

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]

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]

I am able to reproduce it and will try to look into it, hopefully today.

Comment 9

[Nickolay_Ponomarev]

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]

Attachment: call stack for the removeChild call in the <toolbar> constructor

Comment 11

[Nickolay_Ponomarev]

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]

(And I have no idea why webdeveloper is necessary to reproduce this.)

Comment 13

[Nickolay_Ponomarev]

Attachment: 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 [:bzbarsky]]

Attachment: 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 [:bzbarsky]]

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) No longer reading bugmail consistently]

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 [:bzbarsky]]

Jonas, thanks for checking in the fix!

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 261743 [details] [diff] [review]
Proposed patch

Need this to land bug 267833

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 261743 [details] [diff] [review]
Proposed patch

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 20

[Boris Zbarsky [:bzbarsky]]

I've rolled the merged-to-branch version of this into the patch for bug 267833.

Comment 21

[Daniel Veditz [:dveditz]]

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]]

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]]

We're not going to take the regression risk from the XBL re-write on the 1.5 product line.