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

Status: VERIFIED FIXED

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

Description

[Nils Maier [:nmaier]]

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

[Neil Deakin]

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

Comment 2

[Nils Maier [:nmaier]]

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

Comment 3

[Nils Maier [:nmaier]]

Attachment: patch against CVS trunk

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

[Neil Deakin]

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.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase

I'm seeing this crash when dragging the plugin placeholder in this case.

Comment 6

[Martijn Wargers (dead)]

I'm seeing this crash with current trunk build, which means the patch for bug 439565 didn't fix this.

Comment 7

[Tony Chung [:tchung]]

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.

Comment 9

[Tony Chung [:tchung]]

flagging for beta 1 blocker.   More information in bug 458048.

Comment 10

[Boris Zbarsky [:bzbarsky]]

So... I agree that this should only be an issue if SelectNode() fails. Why is it failing at all, though?

Comment 13

[Nils Maier [:nmaier]]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

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.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

This fixes bug 458048 as well. Callers already handle failure so bailing out here shouldn't be a problem.

Comment 18

[Grey Hodge (jX)]

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.

Comment 19

[Dave Townsend [:mossop]]

Resetting blocking

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Attachment: standalone crashtest (requires privileges)

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

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I landed this: http://hg.mozilla.org/mozilla-central/rev/1630d60e624e

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 23

[Nils Maier [:nmaier]]

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.

Comment 24

[:Gavin Sharp [email: [e-mail]]]

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

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Attachment: 1.9.0.x patch

Nils: can you test that this fixes the bug for you on CVS trunk?

Comment 26

[IU]

While the crash no longer happens, there is a regression in that D&D tabs are reloading and they shouldn't be.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Please file a new bug? Commenting in a resolved bug to report new issues is almost never the right thing to do :)

Comment 28

[Hideo Oshima]

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

Comment 29

[Marcia Knous [:marcia]]

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.

Comment 30

[Samuel Sidler (old account; do not CC)]

How often is this crash? Are there known regressions that will need to get fixed if we take this on the stable branch?

Comment 31

[Nils Maier [:nmaier]]

(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

[ar]

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.

Comment 33

[Bill Gianopoulos [:WG9s]]

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

Comment 34

[Boris Zbarsky [:bzbarsky]]

Bill: on 1.9.0 branch?  That's the only thing Samuel is interested here.

Comment 35

[Bill Gianopoulos [:WG9s]]

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

Comment 36

[Samuel Sidler (old account; do not CC)]

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?

Comment 37

[Samuel Sidler (old account; do not CC)]

Minusing per my comment 36 (for release-drivers).

Comment 38

[Daniel Veditz [:dveditz]]

Comment on attachment 341888 [details] [diff] [review]
1.9.0.x patch

Approved for 1.9.0.4, a=dveditz for release-drivers

Comment 39

[:Gavin Sharp [email: [e-mail]]]

mozilla/layout/base/nsPresShell.cpp 	3.1119

Comment 40

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp 	1.22

Comment 41

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #40)
> mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp     1.22

Er, sorry for the spam, commented on the wrong bug.

Comment 42

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 43

[Nils Maier [:nmaier]]

Attachment: branch testcase 1/2: xbl binding to trigger the bug. Based on DownThemAll! code.

Comment 44

[Nils Maier [:nmaier]]

Attachment: branch testcase 2/2: some xul using the xbl. Based on DownThemAll! code.2

Comment 45

[Nils Maier [:nmaier]]

Attachment: branch testcase: Easy to test XPI

Comment 46

[Nils Maier [:nmaier]]

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

Comment 47

[Al Billings [:abillings - ex-MoCo]]

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.