Closed Bug 539531 Opened 13 years ago Closed 13 years ago

Crash when I click-and-drag on a "Gordon" flash emulation [@ nsFrameSelection::ConstrainFrameAndPointToAnchorSubtree(nsIFrame*, nsPoint&, nsIFrame**, nsPoint&) ]

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: dholbert, Assigned: masayuki)

References

()

Details

(Keywords: crash, crashreportid, regression)

Crash Data

Attachments

(3 files, 4 obsolete files)

Attached file backtrace
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20100113 Minefield/3.7a1pre

STEPS TO REPRODUCE:
1. Load URL
2. Click and drag, anywhere in the image.
--> CRASH

Crash reports using an optimized nightly build:
http://crash-stats.mozilla.com/report/index/f7eda297-5781-4c44-8265-5037e2100113
http://crash-stats.mozilla.com/report/index/77b83c24-b407-48a8-86a9-fd7dc2100113

In a debug build, I get this warning just before the crash:
WARNING: NS_ENSURE_TRUE(GetCurrentDoc() == aPresShell->GetDocument()) failed: file ../../../../mozilla/content/base/src/nsGenericElement.cpp, line 397
(In reply to comment #0)
> 2. Click and drag, anywhere in the image.

Actually, some areas of the image don't trigger the crash when dragged.  e.g. if I click on a gray area of road and then drag anywhere, I don't crash.  (Perhaps that's because the road is the background or something?

Most of the image still seems "dangerous" to drag, though. FWIW, here are a few areas of the image that reliably reproduce this for me, when clicked and dragged:
 - The back window of the truck
 - The center of the road (the part with yellow stripes going over it)
 - The sky (both the upper-left & upper-right corners of the image)

For more info on "Gordon", see:
 http://paulirish.com/work/gordon/demos/
 http://github.com/tobeytailor/gordon/

The other demos don't reproduce this as easily, but I've managed to crash three times using the "tiger" demo by furiously clicking & dragging.
Just in case the Gordon developer(s) happen to work around this Firefox crash, I'm attaching a snapshot of the current "Gordon" source (including the "trip" demo), so that we can still debug what's going on, on our end.

This is just a tarball of the folder generated by running:
  git clone git://github.com/tobeytailor/gordon.git

When I view demos/trip.html on my local machine, I get a blank page, but if I move the extracted folder to a web server (people.mozilla.org in my case), the animation plays (and lets me crash Firefox).
No crash in:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.6) Gecko/20091201 Firefox/3.5.6
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2pre) Gecko/20100106 Namoroka/3.6pre

--> Looks like a regression in mozilla-central since 3.6 branched off.
Pretty recent regression, actually.

* NO CRASH:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20100107 Minefield/3.7a1pre
* CRASH:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20100108 Minefield/3.7a1pre
* Guilty pushlog: 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=abb82f981e02&tochange=99bb0c6877f0
In that pushlog, bug 534226 looks most suspicious to me, based on the backtrace & warning message in comment 0.

Tentatively marking as blocking that bug.
Blocks: 534226
Apologies -- from targeted backouts, looks like this is actually a regression from Bug 538466.  Note that the NS_ENSURE_TRUE in comment 0 was actually added in Bug 538466, and that's most likely what caused this.

In a debug build from before Bug 538466's change, I get this output when clicking and dragging the "trip" demo:
###!!! ASSERTION: Wrong root: '!aRoot || (nsContentUtils::ContentIsDescendantOf(aStartN, aRoot) && nsContentUtils::ContentIsDescendantOf(aEndN, aRoot) && aRoot == IsValidBoundary(aStartN) && aRoot == IsValidBoundary(aEndN))', file ../../../../mozilla/content/base/src/nsRange.cpp, line 444
###!!! ASSERTION: Wrong root: '!aRoot || (nsContentUtils::ContentIsDescendantOf(aStartN, aRoot) && nsContentUtils::ContentIsDescendantOf(aEndN, aRoot) && aRoot == IsValidBoundary(aStartN) && aRoot == IsValidBoundary(aEndN))', file ../../../../mozilla/content/base/src/nsRange.cpp, line 444
###!!! ASSERTION: Wrong document somewhere: 'GetCurrentDoc() == aPresShell->GetDocument()', file ../../../../mozilla/content/base/src/nsGenericElement.cpp, line 400
###!!! ASSERTION: unexpected disconnected nodes: 'aDisconnected', file ../../../../mozilla/content/base/src/nsContentUtils.cpp, line 1703
###!!! ASSERTION: unexpected disconnected nodes: 'aDisconnected', file ../../../../mozilla/content/base/src/nsContentUtils.cpp, line 1703

Note that the "Wrong document somewhere" assertion is what Bug 538466 converted into an NS_ENSURE_TRUE, and that change appears to have triggered this crash.
Blocks: 538466
No longer blocks: 534226
Thank you for looking for the cause.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attached patch Patch v1.0 (obsolete) — Splinter Review
Um... I failed to create automated test because I cannot reproduce the crash with nsIDOMNode::removeChild().

The all callers of nsINode::GetSelectionRootContent() should check whether the content is in document.

And they want non-null result for it, so, I added NS_ENSURE_TRUE after nsINode::GetSelectionRootContent().
Attachment #421595 - Flags: review?(bzbarsky)
But the whole point was we changed GetSelectionRootContent() to not assume the input is in the right document.  So the checks for that in the callers should not be needed, should they?

We do need the return value check.
(In reply to comment #9)
> But the whole point was we changed GetSelectionRootContent() to not assume the
> input is in the right document.  So the checks for that in the callers should
> not be needed, should they?

But if the callers send the removed content, GetSelectionRootContent() outputs the warning message on debug build, is it ok?
I think it is, yes.  If we don't want that, we should just not use the NS_ENSURE_* macro in the function.
Attached patch Patch v2.0 (obsolete) — Splinter Review
ok, GetSelectionRootContent() shouldn't output the warning, the callers should out put it if it's unexpected.
Attachment #421595 - Attachment is obsolete: true
Attachment #421771 - Flags: review?(bzbarsky)
Attachment #421595 - Flags: review?(bzbarsky)
Attached patch Patch v2.0.1 (obsolete) — Splinter Review
Sorry, for the spam. The previous patch includes unnecessary change.
Attachment #421771 - Attachment is obsolete: true
Attachment #421774 - Flags: review?(bzbarsky)
Attachment #421771 - Flags: review?(bzbarsky)
Comment on attachment 421774 [details] [diff] [review]
Patch v2.0.1

>+++ b/content/events/src/nsIMEStateManager.cpp
>-  if (!mRootContent && aNode->IsNodeOfType(nsINode::eDOCUMENT)) {
>+  if (!mRootContent || aNode->IsNodeOfType(nsINode::eDOCUMENT)) {

That change doesn't look right.

Looks good other than that.
Attached patch Patch v2.1 (obsolete) — Splinter Review
You're right. mRootContent is already checked at next line.
Attachment #421774 - Attachment is obsolete: true
Attachment #421952 - Flags: review?(bzbarsky)
Attachment #421774 - Flags: review?(bzbarsky)
Attached patch Patch v.2.1.1Splinter Review
oops, sorry for the spam.
Attachment #421952 - Attachment is obsolete: true
Attachment #421957 - Flags: review?(bzbarsky)
Attachment #421952 - Flags: review?(bzbarsky)
Comment on attachment 421957 [details] [diff] [review]
Patch v.2.1.1

r=bzbarsky
Attachment #421957 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
OS: Linux → All
Hardware: x86 → All
http://hg.mozilla.org/mozilla-central/rev/63caa8aa0fab
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Crash Signature: [@ nsFrameSelection::ConstrainFrameAndPointToAnchorSubtree(nsIFrame*, nsPoint&, nsIFrame**, nsPoint&) ]
You need to log in before you can comment on or make changes to this bug.