18102 - [CRASH] Crash clicking in plaintext edit window

Status: VERIFIED FIXED

(Core :: XUL, defect, P1)

Description

[Akkana Peck]

New today: run -edit.  Bring up Debug->Plaintext Editor.  Click anywhere in the
text.  Get a crash.  It seems to be trying to update its bold button, which it
doesn't have in the plaintext edit window.  Looking at Bonsai for recent
changes, I'm guessing this might be related to Hangas' changes, but cc'ing
cmanske since he might know why we're trying to update a nonexistant bold
button.

#0  0x188 in ?? ()
#1  0x40845e5e in nsXULElement::SetAttribute (this=0x8642ba0,
    aName=@0xbfffec8c, aValue=@0xbfffecf4) at nsXULElement.cpp:996
#2  0x413b22b8 in nsInterfaceState::SetNodeAttribute (this=0x871e5b0,
    nodeID=0x413dda39 "Editor:Bold", attributeName=0x413dda34 "bold",
    newValue=@0xbfffecf4) at nsInterfaceState.cpp:398
#3  0x413b1d9a in nsInterfaceState::UpdateTextState (this=0x871e5b0,
    tagName=0x413dda45 "b", observerName=0x413dda39 "Editor:Bold",
    attributeName=0x413dda34 "bold", ioState=@0x871e5c4)
    at nsInterfaceState.cpp:345
#4  0x413b1158 in nsInterfaceState::ForceUpdate (this=0x871e5b0)
    at nsInterfaceState.cpp:140
#5  0x413b110b in nsInterfaceState::NotifySelectionChanged (this=0x871e5b0)
    at nsInterfaceState.cpp:128
#6  0x410827a0 in nsRangeList::NotifySelectionListeners (this=0x8705ff8)
    at nsRangeList.cpp:1328
#7  0x41081f54 in nsRangeList::TakeFocus (this=0x8705ff8, aNewFocus=0x872b5dc,
    aContentOffset=313, aContentEndOffset=313, aContinueSelection=0,
    aMultipleSelection=0) at nsRangeList.cpp:1137
#8  0x410818f2 in nsRangeList::HandleClick (this=0x8705ff8,
    aNewFocus=0x872b5dc, aContentOffset=313, aContentEndOffset=313,
    aContinueSelection=0, aMultipleSelection=0, aHint=0)
    at nsRangeList.cpp:1029

Comment 1

[Chris Waterson]

is this linux-only? i am not seeing this problem on win32. also, i doubt that
hangas is the best person to be looking at this bug, but maybe i'm missing
something?

Comment 2

[Chris Waterson]

looks like an XP XUL bug.

Comment 3

[Chris Waterson]

taking from hangas

Comment 4

[Chris Waterson]

ok, what's happening here is that a broadcaster is being hooked up to a
listener node that is contained in an overlay. The overlay is never resolved
into the content model, so it's discarded; i.e., the root node of the overlay
is released. This causes the listener to be destroyed. Since the broadcaster
only has a weak reference to the listener, it is left with a dangling pointer.

I think we are too eagerly adding elements from an overlay to the XUL
document's element map. We should wait until the overlay is resolved. Hyatt,
does this fix sound reasonable to you?

Comment 5

[Charles Manske]

This issues should be fixed, but the editor should suppress the
nsInterfaceState's ForceUpdate call when we are editing text, since there's no
attributes to update!

Comment 6

[Chris Waterson]

okay, the fix for this is getting pretty involved. i've found a bunch of
commonalities between nsXULDocument::AddSubtreeToDocument(),
ProcessCommonAttributes(), OverlayForwardReference::Merge(), and
BroadcasterHookup::Resolve(). I'm refactoring this all into
AddSubtreeToDocument(), and just using that instead.

I'm worried about causing regressions, and would like to do the dumb thing
for M11 (and make editor just not set 'bold' for the plaintext editor), and
then land the XUL fix in M12.

Does this sound ok?

Comment 7

[Akkana Peck]

That's okay with me as long as we can work out a way of figuring out from
nsInterfaceState::ForceUpdate() whether we're in a plaintext editor.  There
doesn't seem to be a way of getting the editor flags from outside nsHTMLEditor.
Charley?

Comment 8

[Chris Waterson]

Attachment: proposed fix (may require dos2unix to apply on unix)

Comment 9

[Akkana Peck]

Yes, that patch fixes the problem, and I can use the plaintext editor again, and
updating of the status bars still works in the html editor window.

Comment 10

[Chris Waterson]

I am seeing some command updating regressions in addressbook, but after talking
with hangas, I believe that these are -not- related to this change. I'll go bug
chofmann to get this fix in if somebody'll find hyatt and get him to review it.
:-)

Comment 11

[Syd Logan]

Waterson's patch for this fixes IM crasher bug 369026

Comment 12

[Paul MacQuiddy]

setting sujay as QA contact

Comment 13

[Chris Waterson]

fix checked in, r=saari

Comment 14

[sujay]

verified in 12/7 build.

Comment 15

[leger]

BULK MOVE: Changing component from XUL to XP Toolkit/Widgets: XUL.  XUL 
component will be deleted.

Comment 16

[Pulsebot]

Pushed by ffxbld-merge:
https://hg.mozilla.org/releases/mozilla-beta/rev/590765794597
[fenix] For https://github.com/mozilla-mobile/fenix/issues/18102: Temporary ignore failing test.
https://hg.mozilla.org/releases/mozilla-beta/rev/ed7dc63db981
[fenix] Closes https://github.com/mozilla-mobile/fenix/issues/18102: remove intermittent failing App onCreate duration measurement test.