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

VERIFIED FIXED in mozilla1.9.1a2

Status

()

Core
XUL
--
critical
VERIFIED FIXED
9 years ago
6 years ago

People

(Reporter: Christophe Charron, Assigned: mikekap)

Tracking

(4 keywords)

1.9.0 Branch
mozilla1.9.1a2
x86
All
crash, testcase, verified1.9.0.12, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.0.12 +
wanted1.9.0.x +
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] requires bug 492037, crash signature, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

9 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

9 years ago
Component: General → XP Toolkit/Widgets: Trees
Keywords: crash, testcase
Product: Firefox → Core
QA Contact: general → xptoolkit.trees

Updated

9 years ago
Summary: crash when closing open rdf files or right click on tree after opening rdf files → crash when closing open rdf files or right click on tree after opening rdf files [@ nsXULTemplateQueryProcessorRDF::CheckIsSeparator]
(Reporter)

Updated

9 years ago
Severity: critical → blocker
Created attachment 328514 [details]
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.
Created attachment 328516 [details] [diff] [review]
Patch

The aforementioned patch. 

I'm not entirely sure what conflicts this may cause, but this does seem fix the stale rows.
Attachment #328516 - Flags: review?(hyatt)
Attachment #328516 - Flags: review?(hyatt) → review?(enndeakin)

Comment 4

9 years ago
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.
Attachment #328516 - Flags: review?(enndeakin) → review-
Created attachment 328614 [details] [diff] [review]
Patch v2

Addressing Neil's comments.
Attachment #328516 - Attachment is obsolete: true
Attachment #328614 - Flags: review?(enndeakin)

Comment 6

9 years ago
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.
Attachment #328614 - Flags: review?(enndeakin) → review+
Created attachment 329614 [details] [diff] [review]
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.
Attachment #328614 - Attachment is obsolete: true
Attachment #329614 - Flags: review?(enndeakin)

Updated

9 years ago
Attachment #329614 - Flags: review?(enndeakin) → review+
Attachment #329614 - Flags: superreview?(jst)

Updated

9 years ago
Attachment #329614 - Flags: superreview?(jst) → superreview+

Updated

9 years ago
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
Status: NEW → ASSIGNED
Keywords: checkin-needed
Whiteboard: checkin-needed
Assignee: nobody → mike.kaplinskiy
Severity: blocker → critical
Status: ASSIGNED → NEW
Whiteboard: checkin-needed
Version: unspecified → 1.9.0 Branch
Pushed as 17107:78ac7c4b0cf1.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a2

Comment 9

8 years ago
1.9.0 !exploitable report
UNKNOWN: Read Access Violation starting at xpcom_core!PL_DHashTableOperate
Flags: in-testsuite+
Flags: blocking1.9.0.11?
Group: core-security
Flags: wanted1.9.0.x+
Whiteboard: [sg:critical?]
Flags: blocking1.9.0.11? → blocking1.9.0.11+
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 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.
Attachment #329614 - Flags: review?(enndeakin)

Comment 12

8 years ago
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.
Assignee: mike.kaplinskiy → enndeakin
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.
Assignee: enndeakin → mike.kaplinskiy
Flags: blocking1.9.1?

Updated

8 years ago
Depends on: 492037

Comment 14

8 years ago
I filed bug 492037 on the regression.

Updated

8 years ago
Flags: blocking1.9.1?
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).
Flags: blocking1.9.0.11+ → blocking1.9.0.12+
Whiteboard: [sg:critical?] → [sg:critical?] requires bug 492037

Updated

8 years ago
Depends on: 495077

Updated

8 years ago
Attachment #329614 - Flags: review?(enndeakin) → review+

Comment 16

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

Updated

8 years ago
Keywords: fixed1.9.0.12
Was this fixed by bug 492037 or did you land this patch on 1.9.0 as well?
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.
Keywords: fixed1.9.0.12 → verified1.9.0.12
Keywords: fixed1.9.1

Comment 19

8 years ago
It doesn't seem to affect 1.8.
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Flags: wanted1.8.1.x-
Group: core-security
Crash Signature: [@ nsXULTemplateQueryProcessorRDF::CheckIsSeparator]
You need to log in before you can comment on or make changes to this bug.