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

RESOLVED FIXED

Status

()

Core
Editor
--
critical
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: smaug)

Tracking

({crash, regression, testcase})

Trunk
x86
Windows XP
crash, regression, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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).
(Reporter)

Comment 1

12 years ago
Created attachment 220418 [details]
testcase

Talkback ID: TB18174119E

Comment 2

12 years ago
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".
(Reporter)

Comment 3

12 years ago
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  

Updated

12 years ago
Summary: Crash when releasing mouse while dragging absolute positioned element in designMode → Crash when releasing mouse while dragging absolute positioned element in designMode [@ nsHTMLEditor::MouseUp]
QA Contact: editor
Assignee: mozeditor → nobody
(Reporter)

Comment 4

11 years ago
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?
(Assignee)

Comment 5

11 years ago
I can reproduce, taking.
Status: NEW → ASSIGNED
(Assignee)

Updated

11 years ago
Assignee: nobody → Olli.Pettay
Status: ASSIGNED → NEW
(Assignee)

Comment 6

11 years ago
Created attachment 264234 [details] [diff] [review]
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.
(Reporter)

Comment 7

11 years ago
Yeah, the patch fixes the testcase and the mozillazine-testcase here, thanks.
(Assignee)

Updated

11 years ago
Attachment #264234 - Flags: superreview?(neil)
Attachment #264234 - Flags: review?(neil)

Comment 8

11 years ago
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?
Attachment #264234 - Flags: superreview?(neil)
Attachment #264234 - Flags: review?(neil)
(Assignee)

Updated

11 years ago
Attachment #264234 - Flags: superreview?(daniel)
Attachment #264234 - Flags: review?(daniel)
(Assignee)

Updated

11 years ago
Attachment #264234 - Flags: superreview?(daniel)
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...)
(Assignee)

Comment 10

11 years ago
(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.
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
Attachment #264234 - Flags: review?(daniel) → review-
(Assignee)

Comment 12

11 years ago
Created attachment 267565 [details] [diff] [review]
like this?
Attachment #264234 - Attachment is obsolete: true
Attachment #267565 - Flags: review?(daniel)
(Assignee)

Updated

11 years ago
Attachment #267565 - Flags: superreview?(peterv)
Comment on attachment 267565 [details] [diff] [review]
like this?

Yes, exactly like this.
Thanks smaug...

r=daniel@glazman.org
Attachment #267565 - Flags: review?(daniel) → review+
Attachment #267565 - Flags: superreview?(peterv) → superreview+
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite?
(Reporter)

Comment 14

11 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

11 years ago
That is different crash, different stack and all...
Martijn could you file a new bug?
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

11 years ago
The new crash should be fixed by the patch in bug 382778.
(Reporter)

Comment 17

11 years ago
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.
Crash Signature: [@ nsHTMLEditor::MouseUp]
You need to log in before you can comment on or make changes to this bug.