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

VERIFIED FIXED in mozilla1.9.1b1

Status

()

defect
--
blocker
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: nmaier, Assigned: Gavin)

Tracking

(4 keywords)

Trunk
mozilla1.9.1b1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
blocking1.9.0.4 -
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(6 attachments, 2 obsolete attachments)

Reporter

Description

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

Comment 1

11 years ago
Looks like a duplicate of 439565. Can you confirm that the patch there fixes this?
Reporter

Comment 2

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

Comment 3

11 years ago
Posted 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. ;)

Comment 4

11 years ago
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.
Posted 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.

Updated

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

Updated

11 years ago
Duplicate of this bug: 458094

Updated

11 years ago
Duplicate of this bug: 458094
Reporter

Comment 13

11 years ago
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?
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.
Posted 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)

Updated

11 years ago
Status: ASSIGNED → NEW
Flags: blocking1.9.1+ → blocking1.9.1?

Comment 18

11 years ago
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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Reporter

Comment 23

11 years ago
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).
Posted patch 1.9.0.x patchSplinter Review
Nils: can you test that this fixes the bug for you on CVS trunk?

Comment 26

11 years ago
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 :)

Comment 28

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

Comment 31

11 years ago
(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.

Comment 32

11 years ago
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.
Reporter

Comment 46

11 years ago
(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.

Updated

11 years ago
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.