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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: 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

Comment 1

[Daniel Holbert [:dholbert]]

(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.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: tar.gz snapshot of "gordon" src (including demos)

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).

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Daniel Holbert [:dholbert]]

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.

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thank you for looking for the cause.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

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().

Comment 9

[Boris Zbarsky [: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.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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?

Comment 11

[Boris Zbarsky [:bzbarsky]]

I think it is, yes.  If we don't want that, we should just not use the NS_ENSURE_* macro in the function.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

ok, GetSelectionRootContent() shouldn't output the warning, the callers should out put it if it's unexpected.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0.1

Sorry, for the spam. The previous patch includes unnecessary change.

Comment 14

[Boris Zbarsky [: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.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.1

You're right. mRootContent is already checked at next line.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v.2.1.1

oops, sorry for the spam.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 421957 [details] [diff] [review]
Patch v.2.1.1

r=bzbarsky

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/63caa8aa0fab