156719 - Crash on File Bookmark with multiple tabs open

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect, P1)

Description

[WD]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.1a+) Gecko/20020710
BuildID:    2002071008

If mozilla has more than one tab showing, clicking Bookmarks -> File Bookmark
crashes Mozilla.

Reproducible: Always
Steps to Reproduce:
1.Hit Crtl-T to open a new tab
2.Click Bookmarks -> File Bookmark
3.

Actual Results:  Mozilla crashes

Expected Results:  no crash

TB8182363Q
TB8182299G
etc...

Comment 1

[Tuukka Tolvanen (sp3000)]

linux trunk cvs 2002-07-10 18(+03) crashes, 2002-07-08 23(+03) wfm. os->all

Comment 2

[Tuukka Tolvanen (sp3000)]

Attachment: gdb bt debug trunk cvs 2002-07-10

2002-07-09 18(+03) doesn't crash.
2002-07-09 08:00 -- 2002-07-10 08:00 pdt, only checkin with "bookm" is bug
144992

Comment 3

[Tuukka Tolvanen (sp3000)]

nsCSSFrameConstructor bug 143862
nsGlobalWindow bug 156248 bug 134315
nsWebShell bug 44887

Comment 4

[Tuukka Tolvanen (sp3000)]

Looks like backing out bug 143862 fixes this crash, cc dbaron

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to prevent a ReconstructDocElementHierarchy (reframe of the doc element's frame) from crashing for XUL when popups are involved

This fixes a crash when doing a reconstruct-doc-element-hierarchy on a XUL
document.  I suspect it fixes bug 136513 as well, but I haven't checked yet.

I still haven't figured out why we decide this style change is worth a reframe.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Taking

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to prevent a reframe for style attribute change unless it's needed

This patch prevents the reframe -- either of the two patches fixes this crash,
although only the first would fix the nsEngineer panel bug, and they're both
correct and should be checked in.

This fixes nsXULElement::GetMappedAttributeImpact not to return a framechange
hint for the style attribute.  This will cause
nsCSSFrameConstructor::AttributeChanged, the only caller of that method on XUL
elements, to do compute the style change and take the appropriate action based
on the differences.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 90862 [details] [diff] [review]
patch to prevent a ReconstructDocElementHierarchy (reframe of the doc element's frame) from crashing for XUL when popups are involved

sr=ben

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 90870 [details] [diff] [review]
patch to prevent a reframe for style attribute change unless it's needed

sr=ben

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: updated version of testcase from bug 41534

This is an updated version of the testcase from bug 41534 to show that this
change doesn't regress bug 41534 (which is what the code removed by attachment
90870 [details] [diff] [review] was originally put in for, although much of the relevant parts of the
style system have been rewritten since then).

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to prevent reframe for style attribute unless needed and bring XUL style attribute changes up to snuff

This changes WalkInlineStyleRules to GetInlineStyleRules so that
nsCSSFrameConstructor::AttributeChanged doesn't need to be so hacky.  This also
means XUL is included.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch to prevent reframe for style attribute unless needed and bring XUL style attribute changes up to snuff

oops

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Transferring keywords from reopen of bug 143862.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 90909 [details] [diff] [review]
patch to prevent reframe for style attribute unless needed and bring XUL style attribute changes up to snuff

Moving this patch to bug 156971.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Both approaches will fix this bug, but I'll move what has become the more
complex (and less directly related to the crash -- it won't fix the other
occurrences of the same crash) patch into bug 156971, so this bug will now just
cover attachment 90862 [details] [diff] [review].

Comment 16

[Chris Waterson]

Comment on attachment 90862 [details] [diff] [review]
patch to prevent a ReconstructDocElementHierarchy (reframe of the doc element's frame) from crashing for XUL when popups are involved

r=waterson

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 90862 [details] [diff] [review]
patch to prevent a ReconstructDocElementHierarchy (reframe of the doc element's frame) from crashing for XUL when popups are involved

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2002-07-11 18:46 PDT.

I might want to get this on the branch because of bug 136513.

Comment 19

[Matthias Versen [:Matti]]

*** Bug 157029 has been marked as a duplicate of this bug. ***

Comment 20

[Tuukka Tolvanen (sp3000)]

*** Bug 157064 has been marked as a duplicate of this bug. ***

Comment 21

[John Morrison]

*** Bug 157181 has been marked as a duplicate of this bug. ***

Comment 22

[Asa Dotzler [:asa]]

this and bug 143862 were backed out for 1.1 because it caused the problem at bug
157322. 

Comment 23

[ly nguyen]

cc-ing talkback team
zt4newcrash -> blocker

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, please, this was fixed by a backout, long ago.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Except I still need the bug open because it still needs to be fixed so that I
can re-land the other fix.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: slightly cleaner patch

I think I prefer this patch to the other one, since this makes things a little
safer and it also works when someone decides to (incorrectly) stick a
popupgroup element in their XUL document.

This patch, instead of registering the popupset by going down from the top of
the frame tree, walks up two steps in the frame tree to ensure that it's really
a popupset created by the doc element box frame.  It also does unregistration
in a parallel manner, so that there's never a dangling pointer.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In a review request for the new patch, I wrote:

  This is a cleaner version of a patch for a bug that's been around for
  a while, but that my patch for bug 143862 made worse, namely that
  XUL crashes if we have popup stuff in a document that has a frame
  reconstruct done on the root frame.
  
  The problem is that the frame tree looks like this:
  
      viewport <(?) nsRootBoxFrame < nsDocElementBoxFrame
      
  and the nsDocElementBoxFrame, which is what is reconstructed by a
  frame reconstruct for the root node, creates the popupset, but we
  access the popupset via methods on the nsRootBoxFrame.  The old code
  didn't allow the popupset to be replaced.  My previous patch just   
  changed that, but this patch still ensures that there's only one    
  popupset at a time by registering and unregistering the popupset with
  the rootbox frame from the popuset's Init and Destroy methods (rather
  than doing the former from the frame constructor and the latter
  nowhere).  This also ensures that we don't register a popupset created
  if someone puts a popupgroup element in their XUL.

Comment 28

[Brian Ryner (not reading)]

Comment on attachment 95776 [details] [diff] [review]
slightly cleaner patch

r=bryner

Comment 29

[kinmoz]

Comment on attachment 95776 [details] [diff] [review]
slightly cleaner patch

sr=[e-mail]

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in, 2002-08-19 11:29 PDT.