Closed Bug 117141 Opened 23 years ago Closed 23 years ago

Composer crashes when loading pages with an HTML Select element [@FrameManager::GetPrimaryFrameFor]

Categories

(Core :: DOM: Editor, defect, P1)

Product:
Core
Component:
DOM: Editor
Platform:
x86
Windows 98
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.9

People

(Reporter: hwaara, Assigned: kinmoz)

References

Details

(Keywords: crash, Whiteboard: [EDITORBASE+])

Crash Data

Attachments

(4 files)

Testcase
(deleted), text/plain
Details
Minimal Test Case
(deleted), text/html
Details
Smaller Test Case (no table required)
(deleted), text/html
Details
Patch Rev 1 (Mask problem by removing unnecessary style rule)
(deleted), patch
Brade
: review+
sfraser_bugs
: superreview+
Details | Diff | Splinter Review
Open the attach document, and you will crash: nsCOMPtr<nsIContent> prevSibling, parent; rv = aContent->GetParent(*getter_AddRefs(parent)); <----- CRASH HERE! FrameManager::GetPrimaryFrameFor(FrameManager * const 0x05a8aea0, nsIContent * 0x05cdfb80, nsIFrame * * 0x0071efac) line 637 + 33 bytes PresShell::GetPrimaryFrameFor(const PresShell * const 0x05a89c70, nsIContent * 0x05cdfb80, nsIFrame * * 0x0071efac) line 5528 + 32 bytes nsCSSFrameConstructor::RecreateFramesForContent(nsIPresContext * 0x05a86410, nsIContent * 0x05cdfb80, int 0, nsIStyleRule * 0x00000000, nsIStyleContext * 0x00000000) line 11821 nsCSSFrameConstructor::ProcessRestyledFrames(nsCSSFrameConstructor * const 0x05a88200, nsStyleChangeList & {...}, nsIPresContext * 0x05a86410) line 10040 PresShell::ReconstructStyleData(PresShell * const 0x05a89c70, int 0) line 5413 PresShell::StyleSheetDisabledStateChanged(PresShell * const 0x05a89c78, nsIDocument * 0x05a81310, nsIStyleSheet * 0x05d3ea00, int 0) line 5462 nsDocument::SetStyleSheetDisabledState(nsIStyleSheet * 0x05d3ea00, int 0) line 1446 nsHTMLEditor::ApplyDocumentOrOverrideStyleSheet(const nsAString & {...}, int 1, nsICSSStyleSheet * * 0x055cc3d8) line 3316 nsHTMLEditor::ApplyOverrideStyleSheet(nsHTMLEditor * const 0x05cab974, const nsAString & {...}, nsICSSStyleSheet * * 0x055cc3d8) line 3250 nsEditorShell::PrepareDocumentForEditing(nsIDOMWindow * 0x05452944, nsIURI * 0x05a74cd0) line 579 + 85 bytes nsEditorShell::EndPageLoad(nsIDOMWindow * 0x05452944, nsIChannel * 0x05a74a70, unsigned int 0) line 4816 + 27 bytes nsEditorShell::OnStateChange(nsEditorShell * const 0x055cc398, nsIWebProgress * 0x05414964, nsIRequest * 0x05a74a70, int 786448, unsigned int 0) line 4585 nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x05414964, nsIRequest * 0x05a74a70, int 786448, unsigned int 0) line 1110 nsDocLoaderImpl::doStopDocumentLoad(nsIRequest * 0x05a74a70, unsigned int 0) line 750 nsDocLoaderImpl::DocLoaderIsEmpty() line 647 nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x05414954, nsIRequest * 0x05ce8d90, nsISupports * 0x05a86410, unsigned int 0) line 578 nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x054148e0, nsIRequest * 0x05ce8d90, nsISupports * 0x05a86410, unsigned int 0) line 525 + 44 bytes imgRequestProxy::OnStopRequest(nsIRequest * 0x05ce8900, nsISupports * 0x00000000, unsigned int 0) line 369 imgRequest::OnStopRequest(imgRequest * const 0x05ce8a98, nsIRequest * 0x05ce8900, nsISupports * 0x00000000, unsigned int 0) line 618 ProxyListener::OnStopRequest(ProxyListener * const 0x05ce8a50, nsIRequest * 0x05ce8900, nsISupports * 0x00000000, unsigned int 0) line 490 nsFileChannel::OnStopRequest(nsFileChannel * const 0x05ce8908, nsIRequest * 0x05ce87b4, nsISupports * 0x00000000, unsigned int 0) line 481 + 41 bytes nsOnStopRequestEvent::HandleEvent() line 213 nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x05cab634) line 116 PL_HandleEvent(PLEvent * 0x05cab634) line 590 + 10 bytes PL_ProcessPendingEvents(PLEventQueue * 0x00adb110) line 520 + 9 bytes _md_EventReceiverProc(HWND__ * 0x00000d60, unsigned int 54715, unsigned int 0, long 11383056) line 1071 + 9 bytes KERNEL32! bff7363b() KERNEL32! bff94407() 00718b9a()
Attached file Testcase (deleted) —
Open this in Composer, and you will crash.
dbaron, should this bug go into the Style System component?
Keywords: crash
Why does the crash happen? Is |aContent| a valid pointer?
Attached file Minimal Test Case (deleted) —
Here's the minimal test case. <body><table border=1><tr><td> <select></select></td></tr></table></body> If I remove the space or the select inside the table cell, we don't crash. To answer dbaron's question, aContent is not null, but it looks like the node it points to has been deleted, since I see 0xdddddddd when trying to view the contents of the address it's pointing at. By the way the style sheet being applied to the doc at the time of the crash is: chrome://editor/content/EditorOverride.css which can be found at mozilla/editor/ui/composer/content/EditorOverride.css.
Do you see any assertions before the crash?
Nope, no assertions, just a crash. It seems that these rules, in EditorOverlay.css, contribute to the crash: select { -moz-user-input: none !important; } :-moz-display-comboboxcontrol-frame { -moz-user-select: text !important; } :-moz-dropdown-list { -moz-user-select: text !important; }
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.9
An interesting point is that the crash will go away if you remove the space inside the table cell. Is this a CSSParser / HTMLParser bug?
*** Bug 120631 has been marked as a duplicate of this bug. ***
Whiteboard: [EDITORBASE]
Just an update since I've spent some of today looking into this bug. The node that we are trying to get the PrimaryFrame for when we crash is an anonymous content TextNode that belongs to the nsComboBoxControlFrame, specifically, it's the 2nd textnode created during the frame construction of the nsHTMLSelectElement (combo box) object. When composer loads EditorOverlay.css, it triggers a call to PresShell::ReconstructStyleData() which calls frameManager->ComputeStyleChangeFor() on the viewport, this returns a changeList that contains 2 un-refcounted nodes (the anonymous TextNode mentioned above and the nsHTMLSelectElement). It then calls cssFrameConstructor->ProcessRestyledFrames() which runs backwards through the changelist recreating frames ... so the select node is processed before the textnode ... during the recreation of the select node's frames, the textnode gets blown away, so when ProcessRestyledFrames() tries to process the next thing in the changeList (TextNode), we eventually crash in GetPrimaryFrameFor() because we are passing around a deleted pointer. I'm currently exploring why the space before the select, in the test case, makes a difference, and so far I'm finding that in the case where there is *no* space before the select node, the oldContext for the nsComboBoxControlFrame, in FrameManager::ReResolveStyleContext(), contains a nsStyleUserInterface, so when we peak into it in nsStyleContext::CalcStyleDifference() it actually picks up a difference between oldContext and newContext. In the case where there is a space before the select, CalcStyleDifference() says there is no nsStyleUserInterface associated with oldContext, so it bails on making any style ui comparisons, and results in CaptureChange() thinking there is no difference between oldContext and newContext and so it doesn't put the nsHTMLSelectElement into the changeList. Apparently this nsStyleUserInterface exists in the child frames of the nsComboboxControlFrame, so while ReResolveStyleContext() processes the nsComboboxControlFrame's children, some of them get added to the changeList, like the textnode and the nsListControlFrame (who's mContent pointer is the nsHTMLSelectElement).
*** Bug 116052 has been marked as a duplicate of this bug. ***
So the reason why things work when there is no space before the select, is because the editor enables the caret just before it loads the style sheet ... the caret, which is attatched to the select item, then calls into the style system which causes some style data to be resloved which then creates an nsStyleUserInterface() which gets cached and used when nsStyleContext::CalcStyleDifference() is called after the stylesheet is loaded. Here's a snippet of how the nsStyleUserInterface object is being created in the no-space case: nsStyleUserInterface::nsStyleUserInterface(const nsStyleUserInterface & {...}) line 1313 nsRuleNode::ComputeUIData(nsStyleStruct * 0x00000000, const nsCSSStruct & {...}, nsIStyleContext * 0x0443e150, nsRuleNode * 0x0443e0f4, const nsRuleNode::RuleDetail & eRulePartialMixed, int 0) line 2510 + 38 bytes nsRuleNode::WalkRuleTree(nsStyleStructID eStyleStruct_UserInterface, nsIStyleContext * 0x0443e150, nsRuleData * 0x0012ec34, nsCSSStruct * 0x0012ec74, int 1) line 1576 + 40 bytes nsRuleNode::GetUIData(nsIStyleContext * 0x0443e150, int 1) line 1216 + 26 bytes nsRuleNode::GetStyleData(nsStyleStructID eStyleStruct_UserInterface, nsIStyleContext * 0x0443e150, int 1) line 4727 + 20 bytes nsStyleContext::GetStyleData(nsStyleStructID eStyleStruct_UserInterface) line 366 nsIFrame::GetStyleData(nsStyleStructID eStyleStruct_UserInterface, const nsStyleStruct * & 0x044607a8) line 562 + 21 bytes nsCaret::SetupDrawingFrameAndOffset() line 714 nsCaret::DrawCaret() line 905 + 11 bytes nsCaret::StartBlinking() line 470 nsCaret::SetCaretVisible(nsCaret * const 0x04463818, int 1) line 223 + 8 bytes PresShell::SetCaretEnabled(PresShell * const 0x04460118, int 1) line 3015 + 36 bytes nsHTMLEditRules::AfterEdit(nsHTMLEditRules * const 0x043b98bc, int -1, short 0) line 345 nsHTMLEditor::EndOperation(nsHTMLEditor * const 0x042c67f8) line 3852 + 62 bytes nsAutoRules::~nsAutoRules() line 125 nsTextEditRules::CreateBogusNodeIfNeeded(nsISelection * 0x044607a8) line 1244 + 25 bytes nsTextEditRules::Init(nsTextEditRules * const 0x043b98bc, nsPlaintextEditor * 0x042c67f8, unsigned int 0) line 147 + 17 bytes nsHTMLEditRules::Init(nsHTMLEditRules * const 0x043b98bc, nsPlaintextEditor * 0x042c67f8, unsigned int 0) line 230 + 17 bytes nsHTMLEditor::InitRules(nsHTMLEditor * const 0x042c67f8) line 455 + 40 bytes I verified that turning off the caret drawing code, therefore preventing this unexpected resolution of style, causes both test cases (with and without the space) to crash. So it seems the problem stems from the fact that nsStyleContext::CalcStyleDifference() expects both the oldContext and newContext to have an nsStyleUserInterface before it will compare the 2 ... in this particular case, the old one doesn't have an nsStyleUserInterface object, but the new one does. In such a case, shouldn't we try to figure out what type of hint to return based solely on the newContext's nsStyleUserInterface object compared with whatever the defaults should be? I verified in the debugger that returning NS_STYLE_HINT_VISUAL or NS_STYLE_HINT_FRAMECHANGE in the (!oldContext && newContext) prevents the crash.
The last paragraph above should've read: "I verified in the debugger that returning NS_STYLE_HINT_VISUAL or NS_STYLE_HINT_FRAMECHANGE in the (!oldContext && newContext) case, prevents the crash."
After some discussion with kin, it seems like the problem is related to the fact that the select is a mix of anonymous frames that point to the SELECT as their mContent and other that point to other content. This is tricking the code in nsStyleChangeList::AppendChange. I'm assuming that what's happening is that: 1 we don't push a change for the toplevel frame for the select 2 we push a change for a frame inside the select with a content node that is not the select itself (based on the stack in comment 0, a FRAMECHANGE hint which crashes because the FRAMECHANGE for the select actually seems to destroy the anonymous *content* as well (is that true? I'm doubting my theory a little.)) 3 we push a FRAMECHANGE hint change for a frame inside the select whose mContent is the SELECT content node itself. This sequence of events would defeat both our protections against FRAMECHANGE hints destroying frames for which we have hints in the stack: * we don't descend into the child frames of a frame whose hint is FRAMECHANGE (nsFrameManager::ReResolveStyleContext) (I'm not completely sure what crash protection this gives, actually, but hyatt said it fixed some crash.) * we remove any changes already in the change list (which would be processed later since the change list is processed in the reverse order of the hints being added) whose associated content node is the same as one being pushed with a FRAMECHANGE hint (nsStyleChangeList::AppendFrame) The reason this leads to a crash is that we process FRAMECHANGE hints based on the content node and all other hints based on the frame. This means we destroy the *primary* frame for the select because of a FRAMECHANGE hint for one of its children. While it would be nice not to have to do this, it would require rewriting a good bit of frame construction. (It's something to keep in mind if we ever do rewrite it.) Since FRAMECHANGE hints are processed based on the content, and we process the change list end-to-beginning, we would destroy the frame associated with the hint from step (2) above while processing the hint from step (3). I'd think this problem would go away once bryner's SELECT rewrite lands, although that may still be a while. However, a "fix" that would I think fix this symptom but avoid having to deal with the underlying problem (which hopefully we wouldn't have to deal with at all once nsListControlFrame/nsComboboxControlFrame go away) would be loading EditorOverlay.css earlier so that it doesn't cause a re-resolve when it's added. That would also be a perf win (not sure how much) when loading documents in editor. (However, would that still leave the problem for when you switched modes in the editor?)
plussing
Whiteboard: [EDITORBASE] → [EDITORBASE+]
Attached file Smaller Test Case (no table required) (deleted) —
To answer some of dbaron's questions/comments above: - Yes, your items 1-3 are exactly the scenario with the test case. - Loading EditorOverride.css earlier, before the document is done loading, may fix/mask the problem, but as you mentioned, the problem could still exist when switching modes since the editor adds/removes stylesheets between mode switches. In this particular case, the styles that contribute to this bug are in EditorOverride.css which is *never* removed, when switching modes. To add/remove stylesheets, the editor needs a presShell, a doc, and a css loader. Are there necko hooks that guarantee that those things are available before any content has been read in, processed and framed? My concern is that the editor won't get notified till some things have already been framed, which would cause us to see this problem anyways. To fix this bug, it may be enough to just remove the styles on the Select's anonymous frame components in EditorOverride.css. I'll take a look at which ones are needed. I really don't think we need to be able to select any of the text inside the select node. If we can get rid of at least one of them, that would prevent/mask the crash. Cc'ing sfraser since I think he added those style rules during his form element selection changes.
*** Bug 120478 has been marked as a duplicate of this bug. ***
Summary: Crash in stylesheet code if you open this document → Crash in stylesheet code if you open this document [@FrameManager::GetPrimaryFrameFor]
Removing this one style rule from EditorOverride.css is enough to prevent the crash because that means that only one of the Select's child frames has a UI style on it. I tested this with <select> (combobox) and <select size=n> (list) widgets and things seem to select/hilite (in composer) exactly the same way.
Comment on attachment 66217 [details] [diff] [review] Patch Rev 1 (Mask problem by removing unnecessary style rule) r=brade but I'd like to see another patch as well (to fix the underlying problem) :-)
Attachment #66217 - Flags: review+
Comment on attachment 66217 [details] [diff] [review] Patch Rev 1 (Mask problem by removing unnecessary style rule) sr=sfraser for the style change. We really should file a bug for the <select> framing weirdness.
Attachment #66217 - Flags: superreview+
Changing summary from: Crash in stylesheet code if you open this document [@FrameManager::GetPrimaryFrameFor] to: Composer crashes when loading pages with an HTML Select element [@FrameManager::GetPrimaryFrameFor] so that it's easier for QA to find and dup other bugs against this. I'll open a different bug regarding the fact that the nsStyleUserInterface optimization (lazily created) can cause the crash when processing restyled frames, when dealing with content that has a UI Style specified on it and some of it's anonymous content frames.
Summary: Crash in stylesheet code if you open this document [@FrameManager::GetPrimaryFrameFor] → Composer crashes when loading pages with an HTML Select element [@FrameManager::GetPrimaryFrameFor]
It's not the fault of that optimization. That just happens to fix this case, but there are other cases with the SELECT element that it wouldn't fix. The problem is that the nsStyleChangeList::AppendChange mechanism doesn't handle the arrangement of anonymous content in the SELECT.
As usual, dbaron is right. :-) I just remembered that you could reproduce the crash with the following style directly on the select frame's children: :-moz-display-comboboxcontrol-frame { -moz-user-input: none !important; } :-moz-dropdown-list { -moz-user-input: none !important; } In that case, even if there was no optimization it would still crash.
I forgot to mention that I checked in the workaround yesterday morning, into the TRUNK: mozilla/editor/ui/composer/content/EditorOverride.css rev 1.18 I'll close this bug out once I get a chance to file the new one.
I filed bug 123049 to track the fact that nsStyleChangeList::AppendChange mechanism doesn't handle the arrangement of anonymous content in select frames. Closing this bug out as fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Hakan, does this work for you now?
It works fine now - good job Kin!
Status: RESOLVED → VERIFIED
*** Bug 125510 has been marked as a duplicate of this bug. ***
Crash Signature: [@FrameManager::GetPrimaryFrameFor]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: