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.

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

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?)

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

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.

Editor triage team: nsbeta1+/adt1

Ok, It took awhile to narrow it down but I finally have a 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.

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).

thx for the reduced testcase.

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

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

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

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

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

-> default owner

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

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.

-->Adam Lock since this also affects saving Chris--does saving in the browser also crash or only in composer?

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.

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.

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?

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

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

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.

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

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.

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.

Restoring priority / target.

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.

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. ;)

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....:(

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 on attachment 119384 [details] [diff] [review] Proposed patch mmm. hack removal. debug spew killer seems fine too.

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.

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

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.

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

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

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

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