Closed Bug 454324 Opened 13 years ago Closed 13 years ago

Crash when selection-less items are dragged [@ nsINode::GetCurrentDoc() ]

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

defect
Not set
blocker

Tracking

()

VERIFIED FIXED
mozilla1.9.1b1

People

(Reporter: nmaier, Assigned: Gavin)

References

Details

(4 keywords)

Crash Data

Attachments

(6 files, 2 obsolete files)

The crash is caused by not checking for null pointers in PresShell::CreateRangePaintInfo:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsPresShell.cpp&mark=5129#5129
startParent and endParent are both null and hence the crash.

Introduced by a patch to bug 375684.
I don't know anything about this code, so I don't know how to fix it.

DownThemAll! crashes (opening the renaming mask panel and dragging a tag into the textbox).
Not sure if some mozilla product is affected directly.

Stack top:
0  	xul.dll  	nsINode::GetCurrentDoc  	
1 	xul.dll 	PresShell::CreateRangePaintInfo 	
2 	xul.dll 	PresShell::RenderNode 	
3 	xul.dll 	nsBaseDragService::DrawDrag 	
4 	xul.dll 	nsDragService::CreateDragImage 	
5 	xul.dll 	nsDragService::InvokeDragSession 	
6 	xul.dll 	nsBaseDragService::InvokeDragSessionWithImage 	
crash-stats a9fea71c-7d82-11dd-b584-001a4bd43ef6
Looks like a duplicate of 439565. Can you confirm that the patch there fixes this?
(In reply to comment #1)
> Looks like a duplicate of 439565. Can you confirm that the patch there fixes
> this?

Unfortunately it does not.
I did a fresh checkout (CVS trunk for now), applied the patch from bug 439565 and did a build.
Still crashes because startParent and endParent are null. Seems that node->IsInDoc() is indeed true.

Just some info that might be helpful:
The node I pass around is an anonymous (XBL) xul:label that is contained within a xul:popup. a xul:textbox in the same xul document (but not in the popup) is the receiver.
Attached patch patch against CVS trunk (obsolete) — Splinter Review
I don't have a working mercurial/central tree yet.
Here is a patch that will fix the crash for the CVS trunk. Should be working with central as well.

As simple as checking for nsnull. ;)
My reading of the range code suggests that startParent/endParent would only be null if range->SelectNode fails. Perhaps we should just error check the call in PresShell::RenderNode, although this patch is good as an extra check.
Attached file testcase (obsolete) —
I'm seeing this crash when dragging the plugin placeholder in this case.
I'm seeing this crash with current trunk build, which means the patch for bug 439565 didn't fix this.
Keywords: crash, testcase
I confirm the crash on the nightly build on: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20081001 Minefield/3.1b1pre

Crash report and stack at: http://crash-stats.mozilla.com/report/index/7727c9f3-8fcf-11dd-acb4-001a4bd43e5c

My STR was dragging a logged in gmail tab along the tab bar.
Duplicate of this bug: 458048
flagging for beta 1 blocker.   More information in bug 458048.
Flags: blocking1.9.1?
Version: 1.9.0 Branch → Trunk
So... I agree that this should only be an issue if SelectNode() fails. Why is it failing at all, though?
Duplicate of this bug: 458094
Duplicate of this bug: 458094
Just to clarify, I'm crashing when using dTa! on:
Mozilla/5.0 (Windows; U; Windows NT 5.2; de; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3

Which is Firefox STABLE.
Flags: blocking1.9.0.4?
Blocks: 458048
Duplicate of this bug: 458354
Duplicate of this bug: 458371
Flags: blocking1.9.1? → blocking1.9.1+
OK, SelectNode is failing because the node is the root of an anonymous content subtree.  In particular, it's a xul:label when dragging the tabs on trunk.

So we should probably do something sane here (like bail out?) on SelectNode failure.  I'd prefer that to the null-checks, I think.
Attached patch patchSplinter Review
This fixes bug 458048 as well. Callers already handle failure so bailing out here shouldn't be a problem.
Assignee: nobody → gavin.sharp
Attachment #337681 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #341731 - Flags: superreview?(bzbarsky)
Attachment #341731 - Flags: review?(bzbarsky)
Status: ASSIGNED → NEW
Flags: blocking1.9.1+ → blocking1.9.1?
Uhh, thanks to timeless to alert me somehow I changed status and the blocking flag. I haven't a clue how that happened, all I wanted to do was CC. Resetting status but I can't fix the flag. Sorry for that.
Status: NEW → ASSIGNED
Resetting blocking
Flags: blocking1.9.1? → blocking1.9.1+
Attachment #341731 - Flags: superreview?(bzbarsky)
Attachment #341731 - Flags: superreview+
Attachment #341731 - Flags: review?(bzbarsky)
Attachment #341731 - Flags: review+
I don't think this can be an actual crashtest, since there's no way to guarantee that those will run with privileges. I guess I should make this a mochitest...
Attachment #340322 - Attachment is obsolete: true
Flags: in-testsuite?
In case it helps, just before dbaron checked the patch in, I figured out an easy way to reproduce this, STR:

1. Open minefield, for me it loads http://www.mozilla.org/projects/minefield/ in a new tab.
2. Click on the tab in the tab bar.
3. Click on the tab bar.
4. Click on the tab in the tab bar.
5. Crash!

I got a similar stack trace to that above, and the patch above fixed it.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Again, what about trunk?

This is a crash bug.
There is an easy, low risk fix (just checking for null).
And our extension is directly affected by this.
Nils: the bug is still flagged with blocking1.9.0.4?, so you can rest assured that it won't be forgotten for 3.0.x. FIXED just means it's fixed on actual trunk (mozilla-central).
Attached patch 1.9.0.x patchSplinter Review
Nils: can you test that this fixes the bug for you on CVS trunk?
While the crash no longer happens, there is a regression in that D&D tabs are reloading and they shouldn't be.
Please file a new bug? Commenting in a resolved bug to report new issues is almost never the right thing to do :)
(In reply to comment #26)
> While the crash no longer happens, there is a regression in that D&D tabs are
> reloading and they shouldn't be.

I have already filed bug 458696.
verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20081007 Minefield/3.1b1pre and Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre) Gecko/20081007 Minefield/3.1b1pre. Verified using the STR in Comment 22.
Status: RESOLVED → VERIFIED
How often is this crash? Are there known regressions that will need to get fixed if we take this on the stable branch?
(In reply to comment #25)
> Created an attachment (id=341888) [details]
> 1.9.0.x patch
> 
> Nils: can you test that this fixes the bug for you on CVS trunk?

The trunk patch indeed fixes the dTa! crash.

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.0.4pre) Gecko/2008100920 Minefield/3.0.4pre
Build tools
Compiler 	Version 	Compiler flags
cl 	14.00.50727.762 	-TC -nologo -W3 -Gy -Fd$(PDBFILE)
cl 	14.00.50727.762 	-GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fd$(PDBFILE)

Configure arguments
--enable-application=browser --disable-accessibility 


I didn't see any regressions on trunk.
The 1.9.1 regression mentioned (D'n'd of tabs causes reload) doesn't apply to trunk.
mochitest results (regular/browser-chrome) didn't change after applying the patch.

I don't know if anything beside DownThemAll! is affected on moz1.9.0.x.
However, DownThemAll! has about 16M downloads and 2M daily active users ATM. I have no data on how many users actually try to use the D'n'D feature that then crashes.
In the latest:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081009 Minefield/3.1b2pre
everything seems working properly.

It allows to reorganize tabs by dragging them as many times as you wish, no crash happens.
(In reply to comment #30)
> How often is this crash? Are there known regressions that will need to get
> fixed if we take this on the stable branch?

The crash happens every time under Linux.  It also happens if you are just trying to click on the tab and inadvertently move the mouse 1 pixel.
Bill: on 1.9.0 branch?  That's the only thing Samuel is interested here.
(In reply to comment #34)
> Bill: on 1.9.0 branch?  That's the only thing Samuel is interested here.

Silly me.  Of course that is what he was asking about.  I really can;t get it to crash on the branch, even under Linux.  And on the trunk it does not crash as much under Windows as it does on Linux.  Crash reporter stats should tell us how big an issue this is on branch, except that crash reporter still does not work right under 64-bit Linux on AMD64.  But that is what I tested with and I could not get it to crash.
Yeah, ok, crash-stats reports 46 crash reports (for Windows only; but then our crash reporter doesn't ship with distros) with the same stack, or at least the same top of the stack.

http://crash-stats.mozilla.com/report/list?product=Firefox&version=Firefox%3A3.0.3&version=Firefox%3A3.0.4pre&query_search=signature&query_type=contains&query=nsINode%3A%3AGetCurrentDoc&date=&range_value=1&range_unit=weeks&do_query=1&signature=nsINode%3A%3AGetCurrentDoc%28%29

Because it's not a top crasher, I don't think it blocks, but we should consider it for approval.

Gavin, if that patch applies cleanly, can you please request approval on it?
Minusing per my comment 36 (for release-drivers).
Flags: blocking1.9.0.4? → blocking1.9.0.4-
Attachment #341888 - Flags: approval1.9.0.4?
Comment on attachment 341888 [details] [diff] [review]
1.9.0.x patch

Approved for 1.9.0.4, a=dveditz for release-drivers
Attachment #341888 - Flags: approval1.9.0.4? → approval1.9.0.4+
mozilla/layout/base/nsPresShell.cpp 	3.1119
Keywords: fixed1.9.0.4
mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp 	1.22
(In reply to comment #40)
> mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp     1.22

Er, sorry for the spam, commented on the wrong bug.
I can't get this to crash with 1.9.0.3 using the testcase in comment 22. Reading comments, it looks like it doesn't reliably crash on branch.
(In reply to comment #42)
> I can't get this to crash with 1.9.0.3 using the testcase in comment 22.
> Reading comments, it looks like it doesn't reliably crash on branch.

The testcase from comment 22 doesn't crash on branch. However we (DownThemAll!) have some code that triggers the crash I originally reported with this bug.
I don't know if there is some browser/mail feature that is affected too.

I wasn't quiet sure how to attach a branch testcase, as this requires XBL as well as some XUL.
The testcase needs chrome privs to run.
Packaged an XPI; hope this makes it easier to test.

Steps:
1) Drag'n'Drop "drag me" to the "here" textbox.
2) Observe Firefox crashing reliably.
The xpi is pretty sweet. Thanks!

Verified for 1.9.0.4 using it with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4pre) Gecko/2008102706 GranParadiso/3.0.4pre.
Keywords: verified1.9.1
Keywords: fixed1.9.1
Crash Signature: [@ nsINode::GetCurrentDoc() ]
You need to log in before you can comment on or make changes to this bug.