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

RESOLVED FIXED in mozilla1.9alpha5

Status

()

Core
Layout
P1
critical
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: Henrik Gemal, Assigned: bz)

Tracking

({crash, regression, verified1.8.1.8})

Trunk
mozilla1.9alpha5
crash, regression, verified1.8.1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.8 +
blocking1.8.0.14 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: regression from 267833?, crash signature)

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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

11 years ago
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?
Keywords: crash
Summary: crash at nsCSSFrameConstructor::ConstructFrame eded793e → crash at [@ nsCSSFrameConstructor::ConstructFrame] with Web Developer Toolbar
(Reporter)

Comment 2

11 years ago
Created attachment 258534 [details]
Webdeveloper XPI
(Reporter)

Comment 3

11 years ago
Created attachment 258535 [details]
Inhouse XPI

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

Updated

11 years ago
Summary: crash at [@ nsCSSFrameConstructor::ConstructFrame] with Web Developer Toolbar → crash at [@ nsCSSFrameConstructor::ConstructFrame] with "Web Developer Toolbar" and custom XPI
(Reporter)

Comment 4

11 years ago
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 :(
(Reporter)

Comment 5

11 years ago
Created attachment 258538 [details]
minimal XPI to reproduce crash (install on top of webdeveloper)

I've narrowed the XPI down to the bare minimal
Attachment #258535 - Attachment is obsolete: true
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?
(Reporter)

Comment 7

11 years ago
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

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

Comment 9

11 years ago
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.
Keywords: regression

Comment 10

11 years ago
Created attachment 258564 [details]
call stack for the removeChild call in the <toolbar> constructor

Comment 11

11 years ago
Comment on attachment 258534 [details]
Webdeveloper XPI

(Note that you need to bump the maxversion in install.rdf to install on trunk :( )
Attachment #258534 - Attachment description: XPI to install → Webdeveloper XPI

Updated

11 years ago
Attachment #258538 - Attachment description: minimal XPI to reproduce crash → minimal XPI to reproduce crash (install on top of webdeveloper)

Comment 12

11 years ago
(And I have no idea why webdeveloper is necessary to reproduce this.)
Blocks: 267833
Flags: blocking1.9?

Comment 13

11 years ago
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.
Attachment #258534 - Attachment is obsolete: true
Attachment #258538 - Attachment is obsolete: true
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.
Attachment #261743 - Flags: superreview?(jonas)
Attachment #261743 - Flags: review?(jonas)
(Assignee)

Updated

11 years ago
Blocks: 373727, 373944
If someone understands the UI end enough to write a standalone (non-extension) test we could check in, that would rock...
Assignee: nobody → bzbarsky
Flags: in-testsuite?
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: crash at [@ nsCSSFrameConstructor::ConstructFrame] with "Web Developer Toolbar" and custom XPI → [FIX]crash at [@ nsCSSFrameConstructor::ConstructFrame] with "Web Developer Toolbar" and custom XPI
Target Milestone: --- → mozilla1.9alpha4
Flags: blocking1.9? → blocking1.9+
(Assignee)

Updated

10 years ago
Target Milestone: mozilla1.9alpha4 → mozilla1.9alpha5
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.
Attachment #261743 - Flags: superreview?(jonas)
Attachment #261743 - Flags: superreview+
Attachment #261743 - Flags: review?(jonas)
Attachment #261743 - Flags: review+
Jonas, thanks for checking in the fix!
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Flags: blocking1.8.1.5+ → blocking1.8.1.6+
Whiteboard: regression from 267833?
Comment on attachment 261743 [details] [diff] [review]
Proposed patch

Need this to land bug 267833
Attachment #261743 - Flags: approval1.8.1.6?
Attachment #261743 - Flags: approval1.8.0.13?
Flags: blocking1.8.0.13+ → blocking1.8.0.14+
Attachment #261743 - Flags: approval1.8.0.13? → approval1.8.0.14?
Comment on attachment 261743 [details] [diff] [review]
Proposed patch

approved for 1.8.1.7, a=dveditz for release-drivers
Attachment #261743 - Flags: approval1.8.1.7? → approval1.8.1.7+
I've rolled the merged-to-branch version of this into the patch for bug 267833.
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
Keywords: fixed1.8.1.8
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
Keywords: fixed1.8.1.8 → verified1.8.1.8
We're not going to take the regression risk from the XBL re-write on the 1.5 product line.
Flags: blocking1.8.0.14+ → blocking1.8.0.14-
Attachment #261743 - Flags: approval1.8.0.14?
Crash Signature: [@ nsCSSFrameConstructor::ConstructFrame]
You need to log in before you can comment on or make changes to this bug.