336160 - Crash when releasing mouse while dragging absolute positioned element in designMode [@ nsHTMLEditor::MouseUp]

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes Mozilla when following the steps to reprodoce:
- Drag the absolute positioned element somewhere
- Keep the mouse down for at least a second, then release it

This regressed between 2005-02-23 and 2005-02-24, so I guess a regression of bug 209020 (although it doesn't work well before that, but at least it doesn't crash).

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Talkback ID: TB18174119E

Comment 2

[Jesse Ruderman]

WFM Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060501 Minefield/3.0a1

TB18174119E gives me a "DB error".

Comment 3

[Martijn Wargers (dead)]

Now, talkback has processed it:

nsHTMLEditor::MouseUp   nsHTMLEditorMouseListener::MouseUp   nsEventListenerManager::HandleEvent   nsEventTargetChainItem::HandleEvent   nsEventTargetChainItem::HandleEventTargetChain   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventTargetChainItem::CreateChainAndHandleEvent   nsEventDispatcher::Dispatch   PresShell::HandleEventInternal   PresShell::HandlePositionedEvent   PresShell::HandleEvent   nsViewManager::HandleEvent   nsViewManager::DispatchEvent   HandleEvent   nsWindow::DispatchEvent   nsWindow::DispatchMouseEvent  

Comment 4

[Martijn Wargers (dead)]

With the steps to reproduce at http://forums.mozillazine.org/viewtopic.php?t=547434&sid=888e81ac5acfa69795d563d781c42a0f I get the same stacktrace.
Olli, can you reproduce this?
Does this need a similar fix as bug 338129?

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I can reproduce, taking.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch

This fixes the testcase crash, but I'm not quite sure what to do
with the mozillazine-testcase, because I never see "You will see an absolutely positioned div block. ".
Martijn, could you test that.

The patch fixes the crash, but I think the behavior should be a bit different. When grabbing something, the selection should the same, even when DOM is mutated. But that is a different bug.

Comment 7

[Martijn Wargers (dead)]

Yeah, the patch fixes the testcase and the mozillazine-testcase here, thanks.

Comment 8

[[e-mail]]

Comment on attachment 264234 [details] [diff] [review]
possible patch

Sorry, I don't understand the relationship between EndMoving and HideGrabber. Perhaps you should ask glazou for review?

Comment 9

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Ok, here's what's happening here : you're moving a positioned object that has the
focus. Since it has the focus, it has a grabber. But calling insertHTML moves the focus to the newly inserted element while you're moving another one !
So you're trying to show a grabber on an element while the grabber is in use on another one... Since there's only one grabber at a time, BOOM.
Let me investigate a bit after lunch, I'd like to see if your patch is really the correct/best thing to do here.

(I must say I never thought of such an edge case because JS should NOT be available in an editing environement. Editing a document that automatically changes over time poses extremely complex issues. Here we are...)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #9)
> ... But calling insertHTML moves
> the focus to the newly inserted element while you're moving another one !
> So you're trying to show a grabber on an element while the grabber is in use on
> another one... Since there's only one grabber at a time, BOOM.

Exactly. The simplest fix I found for the crash was to clear those two
boolean flags. Because mGrabber is nsnull, mGrabberClicked == PR_TRUE doesn't
make much sense.

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

people, I must say I am very very reluctant to fix this bug. I'm going to do
it because it's a crasher, but this is a can of worms we should not address.
We are Disruptive Innovations spent our lunch time discussing it and we found
so many bad things like this you could do it will be impossible to fix them:

- move a positioned element while JS removes it from the DOM
- move a positioned element while JS sets back its 'position' property
  to 'static'
- move a positioned element while JS sets the display property of the
  element or one of its container to 'none'
- and so on

See the problem ? Now, wrt the current bug, the proposed patch is in my
opinion a crash fix but not the right one. What we should do is probably the
following :

- adapt nsHTMLEditor::CheckSelectionStateForAnonymousButtons to refuse
  changes if an element is currently moving
- call that method at the end of nsHTMLEditor::EndMoving to make sure the
  element that has the focus has its grabber

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: like this?

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 267565 [details] [diff] [review]
like this?

Yes, exactly like this.
Thanks smaug...

r=[e-mail]

Comment 14

[Martijn Wargers (dead)]

I'm still crashing in current trunk builds with the testcase with slightly different steps to reproduce:
- Drag the absolute positioned element until the mouse is out of the iframe
- Wait a little while, then relese the left mouse button
- Click inside the iframe

Result: crash
https://crash-reports.mozilla.com/reports/report/index/7c500fab-1b62-11dc-89f0-001a4bd46e84

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

That is different crash, different stack and all...
Martijn could you file a new bug?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

The new crash should be fixed by the patch in bug 382778.

Comment 17

[Martijn Wargers (dead)]

Ok,(In reply to comment #15)
> That is different crash, different stack and all...
> Martijn could you file a new bug?

Ok, I filed it now as bug 384704.