441785 - crash when closing open rdf files or right click on tree after opening rdf files [@ nsXULTemplateQueryProcessorRDF::CheckIsSeparator]

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Christophe Charron]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9) Gecko/2008052906 Firefox/3.0

a xul window loading 2 rdf files in a tree.

Absolutely no problem with Flock, under windows (Mozilla/5.0 (Windows;
U; Windows NT 5.1; en-US; rv: 1.8.1.14) Gecko/20080530 Firefox/
2.0.0.14 Flock/1.2.1), that is say that I could see a tree that data
in the second file rdf. Similarly, if we turn opens and closes files
via the appropriate buttons, no crash.

But with , FF3, under windows, my tree retains data in the first rdf
and if you play a little with files rdf (open and then close them and
open them by clicking on the tree etc.) on a Most likely to have
crashes.
Under Ubuntu 8.04, I crash , but no proposal of sending report crashes.

Below, links to the first 4 crashes generated by this window. 
http://crash-stats.mozilla.com/report/index/1d65a463-428a-11dd-a516-001321b13766
http://crash-stats.mozilla.com/report/index/915efac7-428a-11dd-921d-001cc45a2ce4
http://crash-stats.mozilla.com/report/index/915efac7-428a-11dd-921d-001cc45a2ce4
http://crash-stats.mozilla.com/report/index/c5253097-428a-11dd-82e6-001321b13766

Reproducible: Always

Steps to Reproduce:
1.Open a new Firefox 3
2.go to http://test03.christophe-charron.org/public/xul/2008_06_21/2008-02-21-test01.php
3.click on button 1 
4.close the window with rdf file
5.right click on tree
6. do it 2 or 3 times if doesn't crash at the first time

Comment 1

[[e-mail]]

I crashed using SeaMonkey 2.0a1pre (Gecko 1.9.0.1pre) just by opening the URL and forcing a repaint. The result's query's processor had been deleted (mQuery->mProcessor points to memory filled with 0xdddddddd). Stack:

gklayout!nsXULTemplateQueryProcessorRDF::CheckIsSeparator+0x36
gklayout!nsXULTemplateResultRDF::GetType+0x40
gklayout!nsXULTreeBuilder::IsSeparator+0xca
gklayout!nsTreeBodyFrame::PaintRow+0x26c
gklayout!nsTreeBodyFrame::PaintTreeBody+0x41d
gklayout!PaintTreeBody+0x22
gklayout!nsDisplayGeneric::Paint+0x33
gklayout!nsDisplayList::Paint+0x3e
gklayout!nsDisplayWrapList::Paint+0x1e
gklayout!nsDisplayClip::Paint+0x58
gklayout!nsDisplayList::Paint+0x3e
gklayout!nsLayoutUtils::PaintFrame+0x40d
gklayout!PresShell::Paint+0x17d
gklayout!nsViewManager::RenderViews+0x9b
gklayout!nsViewManager::Refresh+0x307
gklayout!nsViewManager::DispatchEvent+0x3fc
gklayout!HandleEvent+0x49
gkwidget!nsWindow::DispatchEvent+0xba
gkwidget!nsWindow::DispatchWindowEvent+0x22
gkwidget!nsWindow::OnPaint+0x74a

Comment 2

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Test case

Bug still present on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1a1pre) Gecko/2008070812 Minefield/3.1a1pre .

This is a chrome test, although I have no clue where it goes. The only reason for that is apparently the datasources attribute needs a web server and doesn't work for file:/// uri's.

The crash happens when the XPCOM garbage collector collects the query processor for the first file's results. When the results are then repainted, we get into a mess.

The old results are added to the tree when the datasources attribute is changed twice and two query processors are created, although only one reference is kept. Then when the async data for the stale file comes in, it sometimes ends up being added to the tree anyway.

The fix I found to work (for this case anyway) is to kill the (current) query processor at nsXULTemplateBuilder::LoadDataSources, although I don't know what implications that will have.

Comment 3

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch

The aforementioned patch. 

I'm not entirely sure what conflicts this may cause, but this does seem fix the stale rows.

Comment 4

[Neil Deakin]

Comment on attachment 328516 [details] [diff] [review]
Patch

This should be a call to Uninit(PR_FALSE) to uninitialize things. I think it would be better to call this within AttributeChanged as it doesn't need to be called during Init.

Comment 5

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v2

Addressing Neil's comments.

Comment 6

[Neil Deakin]

Comment on attachment 328614 [details] [diff] [review]
Patch v2

Looks OK, but no observers are removed so just leave out that part of the comment.

You should also add a crashtest.

Comment 7

[Mike Kaplinskiy [:mikekap] (at school, no time)]

Attachment: Patch v3 w/ semi-working crashtest

This crashtest doesn't completely work for two reasons:
 - Apparently strict_origin_policy doesn't allow loading rdf files from the same directory. Is this is a bug? (At least the other tests that load rdf files shouldn't really work)
 - The crash requires a garbage collection, so it doesn't work on release builds. It works fine for debug builds though (and will pass unconditionally on release builds).

I doubt this is good, but I don't think I can get much further than that.

Also fixed a typo.

Comment 8

[Reed Loden [:reed]]

Pushed as 17107:78ac7c4b0cf1.

Comment 9

[Bob Clary [:bc] (inactive)]

1.9.0 !exploitable report
UNKNOWN: Read Access Violation starting at xpcom_core!PL_DHashTableOperate

Comment 10

[Daniel Veditz [:dveditz]]

The patch applies to 1.9.0, but I'm not sure it's working right. With the patch I get a completely empty tree. Pressing the buttons loads the XML in an external window, but the tree in the original window is never populated.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 329614 [details] [diff] [review]
Patch v3 w/ semi-working crashtest

That's the result I get in Shiretoko, too: the test page shows a blank tree instead of being populated by the datasource it set. Did Shiretoko break rdf datasources? If so I would have expected we'd have found out by now.

enn: is this behavior correct? I'm happy to land this patch if it is, but I don't want to break things and force a quick re-release. We've had enough of those lately. requesting re-review in the context of the 1.9.0 branch just as a mechanism to get your input as to whether this is appropriate for that branch.

Comment 12

[Neil Deakin]

Hmmm. It seems that this patch fixes the crash but breaks the actual testcase completely. Fortunately I have a patch in the works which should fix this as well as doesn't bring back the crash.

Comment 13

[Daniel Veditz [:dveditz]]

Are you working on that in an existing bug? If not should I reopen this one or file a new one on the regression? Nominating for blocking1.9.1 so we don't forget the regression, but that request should be moved if there's a separate bug.

Comment 14

[Neil Deakin]

I filed bug 492037 on the regression.

Comment 15

[Daniel Veditz [:dveditz]]

We'll take this fix along with the regression fix in the next 1.9.0 cycle (we're past code-freeze already and would like some nightly testing on the new fix).

Comment 16

[Neil Deakin]

Comment on attachment 329614 [details] [diff] [review]
Patch v3 w/ semi-working crashtest

Once the regression bug is checked in, this should be ok for 1.9.0

Comment 17

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

Was this fixed by bug 492037 or did you land this patch on 1.9.0 as well?

Comment 18

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

Verified that this is fixed in 1.9.0.12 using original testcase on website and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.12pre) Gecko/2009063005 GranParadiso/3.0.12pre (.NET CLR 3.5.30729). 1.9.0.11 crashes following test steps.

Comment 19

[Martin Stránský [:stransky] (ni? me)]

It doesn't seem to affect 1.8.

Comment 20

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090721 Minefield/3.6a1pre ID:20090721044139

and

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090720 Shiretoko/3.5.1pre ID:20090720042942