197942 - [FIX]Saving/Publishing page results in a crash - Trunk [@ nsComboboxControlFrame::ReflowCombobox]

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect, P2)

Description

[Chris Petersen]

Build: 2003-03-13-03
Platform: All
Expected Results: Page should be published to server
What I got: Application crashs during the publishing process
Steps to reproduce:

1) Go to any bugzilla bug report like this one
2) Select Edit Page from File menu
3) Select Publish As
4) Specify a ftp address, site name, page title, and file name.
5) Publish document
6) Publishing to address dialog appears
7) Notice mozilla-banner.gif is transfered but crash occurs while transfer html
and css files.

Comment 1

[Chris Petersen]

Attachment: stack trace

Comment 2

[Chris Petersen]

This problem is also occuring on a win32 trunk build (2003-03-13-04) under
Windows XP.

Comment 3

[Kathleen :Brade]

Chris--What ftp server are you trying to publish to?  Is it possible to reduce
this to a more minimal testcase (can you remove all but a couple things and
still crash?)

Comment 4

[Chris Petersen]

Sure. I'm planning to create a reduced test case of this page.

Comment 5

[Kathleen :Brade]

I'm guessing this is related to or a duplicate of the other ftp bug on Darin's
plate.

220 geckoqa Microsoft FTP Service (Version 5.0).
Remote system type is Windows_NT.

Comment 6

[Samir Gehani]

Editor triage team: nsbeta1+/adt1

Comment 7

[Chris Petersen]

Ok, It took awhile to narrow it down but I finally have a reduced test case :).

Comment 8

[Chris Petersen]

Attachment: Reduced test case

This test case uses a LINK and SELECT elements. The LINK element references the
original css with a absolute path. The SELECT element contains a single OPTION
element. If you open this test case in Composer and publish it, a crash should
occur.

Comment 9

[Chris Petersen]

You can reproduce the crash if both the test case and the css file are stored
locally. The CSS file can actually be a empty file but needs to be in a separate
folder (not in the same directory as the test case). 

Comment 10

[Darin Fisher]

thx for the reduced testcase.

Comment 11

[Darin Fisher]

the stack trace doesn't seem to have anything to do with networking.  cc'ing bz
to help triage.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Is there an FTP server I could use to repro this?

Comment 13

[Peter Lubczynski]

hm...I'm getting a slightly differenet stack in the debugger but the cause of my
crash is because nsRuleNode::mPresContext is a bogus pointer (0xddddd) possibly
because nsStyleContext::mRuleNode is bogus. cc:ing more layout folks

Comment 14

[Boris Zbarsky [:bzbarsky]]

hmm.. style contexts hold strong refs to rulenodes....  Is someone
over-releasing a rulenode somewhere?

Comment 15

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

The nsStyleContext -> nsRuleNode reference is through a GC (see bug 117316), not
reference counting (which it hasn't been since nsIRuleNode was removed).

Comment 16

[Darin Fisher]

-> default owner

Comment 17

[Kathleen :Brade]

Chris--can you confirm that the same crash results if you publish with http or
saveAs to a new location?

Comment 18

[Chris Petersen]

Yes, I get the same stack trace (as attached) when either saving the test case
(Save as)  or using http:// to publish it to the server. Tested with Mach-O
2003-03-20-03 trunk build.

Comment 19

[Kathleen :Brade]

-->Adam Lock since this also affects saving

Chris--does saving in the browser also crash or only in composer?

Comment 20

[Adam Lock]

Attachment: stack trace on Windows XP

I was able to reproduce this problem on XP as well.

It is not obvious what the cause might be, since it isn't happening anywhere in
webbrowser persist, but in the layout code. Perhaps there is a refcount issue
somewhere, either in the layout, or the upload section of persist.

I will see if I can pin this down further.

Comment 21

[Adam Lock]

After talking to Kathy, one avenue of investigation looks promising.

The document is published via the persist object and specifying the
PERSIST_FLAGS_FIXUP_ORIGINAL_DOM flag. This means that during upload the
editor's copy of the dom document is being fixed up and the LINK's href
attribute will be changed to point to the CSS which has just been or is in the
process of being uploaded.

The next time the page is repainted the crash occurs.

It's possible that changing the href on the link might be causing some edge case
issue where style data is not being correctly cleaned out and reloaded. I'll see
if I can confirm this idea.

Comment 22

[Adam Lock]

This bug probably belongs to Boris.

Bug is definitely caused by fixing up the original DOM. If I blank out that flag
uploading happens properly.

In the reduced test case above, upload sets the link element's new href to
"edit_bug.css" which propogates through the DOM code resulting in
nsStyleLinkElement::UpdateStyleSheet being called. This method sees the relative
link and resolves it to "http://bugzilla.mozilla.org/edit_bug.css". At this
point CSSLoaderImpl::LoadStyleLink tries to load the url synchronously, gets an
http error and during or after this it appears to leave the style rules in an
inconsistent state. The next repaint crashes it.

I'm going to reassign to Boris for the time being. It pretty much looks like a
CSS thing at this stage.

Note that even if this bug is fixed, the page css may be broken after upload
since it's resolved incorrectly. Perhaps telling persist to fixup the original
dom is not appropriate when editing non-local files?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> At this point CSSLoaderImpl::LoadStyleLink tries to load the url synchronously

Um?  LoadStyleLink never does synchronous loading...

So there are at least 3 separate bugs here:

1) The wrong url is used (Adam's turf ;) )
2) The sheet is loaded at all
3) We crash.

At a guess, it looks like #2 is caused by the way CloneNode is implemented on
Link elements.  I'd say we want to disable updates before copying over all the
attributes.  Peter?  What do you think?

#3 is most likely caused by bug 85631

In any case, how do I reproduce this?  I do not have access to any FTP servers
allowing upload that I'm aware of.

Comment 24

[Kathleen :Brade]

bz--you don't need to publish, saving locally also crashes (in Composer).

Comment 25

[Chris Petersen]

Kathy- The problem only happens when saving or publishing this test case with
Composer. This same test case is properly saved when using the browser. Tested
with the 2003-04-02-03 Mach0 trunk build.

Comment 26

[Peter Van der Beken [:peterv]]

Is CloneNode really starting the load? IIRC we don't try to load until putting
the node in the document.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Well, I'm confused.  Since PERSIST_FLAGS_FIXUP_ORIGINAL_DOM is set, we don't
clone the node -- we modify it in-place (and yes, CloneNode does not load the
sheet; sorry, Peter).

I have a fix for issue #2 (I _think_ we want to fix it... but do we?). 
Attaching that.  Still looking into issue #3.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: don't load the new sheet....

Comment 29

[Adam Lock]

Perhaps I was confused about the synchronous thing, but I was looking at the
#ifdef ALLOW_ASYNCH_STYLE_SHEETS at the top of nsStyleLinkElement.cpp. Since
that's not defined anywhere I thought the default blocking mode as synchronous.

As for the CloneNode issue, I don't think it matters for persistence since if
you're cloning nodes during persistence you won't be rendering them anyway.

Comment 30

[Adam Lock]

Restoring priority / target.

Comment 31

[Boris Zbarsky [:bzbarsky]]

So what I am seeing is a _single_ call into UpdateStyleSheet, followed by a
crash on the following reflow.  So bug 85631 is not likely to be a problem.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: empty css file

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Really minimal testcase

Seems to only be a problem with comboboxes.... in a debug build I get:

frame: Block(-1) (0x870eaf4) style: 0x870eb74
:-moz-display-comboboxcontrol-frame {}
Wrong parent style context:  style: 0x870e394 {}
should be using:  style: 0x872bf58 {}
Style contexts are not in the same style context tree.

which is never a good sign.  ;)

Comment 34

[Boris Zbarsky [:bzbarsky]]

jkeiser, thoughts?  It looks like the style context for the mDisplayFrame in
nsComboboxControlFrame is just wrong.  Stepping through the code, the reresolve
for that context comes up with a reconstruct hint (!) so the context is left on
the frame... but the frame may never get reconstructed properly... (otherwise
I'd think it would have the right style context).

In any case, do you understand the mDisplayFrame code?  It seems to not call
Destroy() on frames, to not put mDisplayFrame in any child lists (I _did_ try
doing that, and that did not fix the crash), etc....:(

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Fix the block pseudo-frame inside the combobox to really be a pseudo-frame. 
This fixes the ReResolveStyleContext errors I was seeing.... (the blockframe
changes are removal of a hack that this bogosity necessitated, and a snuck-in
fix for some debug spew from the viewmanager).

Comment 36

[John Keiser (jkeiser)]

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

mmm.  hack removal.  debug spew killer seems fine too.

Comment 37

[kinmoz]

bz, here's the bug I mentioned to you yesterday on IRC which sounded familiar
... bug 166596. What I remember seeing was that there was a stale reference to
some style by one of the components in the combobox, and the bug seemed timing
related, that is, a few strategically placed breakpoints would prevent the bug
from happening.

Does your proposed patch also fix this problem? The stack traces are slightly
different.

Comment 38

[Boris Zbarsky [:bzbarsky]]

As a note, the reason things break without this patch is that if the
:-moz-display-comboboxcontrol-frame gets a reconstruct hint we attempt to
reframe it, and hit the following code in nsCSSFrameConstructor::ContentRemoved:

      // For "select" add the pseudo frame after the last item is deleted
      nsIFrame* parentFrame = nsnull;
      childFrame->GetParent(&parentFrame);
      if (parentFrame == selectFrame) {
        return NS_ERROR_FAILURE;
      }

It _seems_ that we get a framechange hint because our -moz-user-input value is
changing from "auto" to "none" or back.  Not sure why that's happening,
exactly...  See discussion in bug 166596.

It's not clear to me why rods added that code, and it seems incredibly wrong to
me.... (xbl form controls, anyone?  ;) )

Comment 39

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

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

If you're going to make the  function inline, please move it into the header. 
(Otherwise you have a pretty good chance of breaking some obscure compilers.)

sr=dbaron if you've tested this after roc landed.

Comment 40

[Boris Zbarsky [:bzbarsky]]

I have indeed.  After his landing we don't even have to save the page -- we
crash just opening it in Composer.  ;)  Fixed by this patch.

Checked in.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 42

[Jay Patel [:jay]]

Adding topcrash info for future reference...this was a recent topcrasher on the
Trunk from 4/9 - 4/12.

Comment 43

[Jay Patel [:jay]]

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

Comment 44

[Chris Petersen]

Verified in the 2003-04-22-08 Macho and Win32 trunk builds.