Crash when closing tabs in cairo-qt

RESOLVED FIXED

Status

Core Graveyard
Widget: Qt
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: Tobias Hunger, Unassigned)

Tracking

({fixed1.9.1})

Trunk
x86
Linux
fixed1.9.1
Bug Flags:
blocking1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 369720 [details] [diff] [review]
cairo-qt tab crash fix.

When closing tabs e.g. in firefox using a cairo-qt enabled xulrunner sometimes triggers a segmentation fault.

The root cause is that the nsWindow assumes that it owns the associated MozQWidget. This is not the case: QWidgets are owned and destroyed by their parents. In some cases the MozQWidget is deleted due to this, before the nsWindow gets destructed. This triggers a segmentation fault when the nsWindow accesses the MozQWidget in Destroy().

The patch adds a method to nsWindow (Qt-flavour) that sets the associated QWidget to 0, preventing the issue. MozQWidget is changed to call this method in its destructor.

The patch touches code used by cairo-qt only, so there should be no repercussions with other toolkit options.
Attachment #369720 - Flags: review?
Comment on attachment 369720 [details] [diff] [review]
cairo-qt tab crash fix.


>+    mMozQWidget = nsnull;
>     mDrawingArea = nsnull;
> 
> 
>+void nsWindow::QWidgetDestroyed()
>+{
>+    mDrawingArea = 0;
>+    mMozQWidget = 0;
>+}
>+

Since we initialize to | nsnull | maybe you should do the same on destruction.
Attachment #369720 - Flags: review? → review?(vladimir)
Tested, with this patch crash disappeared.
Keywords: checkin-needed
Tobias, can you update the patch according to comment #1 so we can get this checked in? Thanks

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: needs updated patch
(Reporter)

Comment 4

9 years ago
Created attachment 380068 [details] [diff] [review]
Updated patch.

This is identical to the previous (approved) patch, with the tiny changes commented on upon.
Attachment #369720 - Attachment is obsolete: true
Attachment #380068 - Flags: review+
(Reporter)

Updated

9 years ago
Whiteboard: needs updated patch
Keywords: checkin-needed
(Reporter)

Comment 5

9 years ago
Ping?

Could somebody please apply this patch?
The tree is closed, see:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox
"
The tree is RESTRICTED  to patches on bugs marked as blocking1.9.1 or blocking-firefox3.5, or for bustage/regression fixes that need to also land on Gecko 1.9.1
"
(Reporter)

Comment 7

9 years ago
The Qt code is in Gecko 1.9.1. This patch fixes a core dump in it. So it is a bustage in Gecko 1.9.1.

Please apply the patch.
Attachment #380068 - Flags: approval1.9.1?
Comment on attachment 380068 [details] [diff] [review]
Updated patch.

This patch fixes a crash in our Qt backend. It's needed on trunk and 191.
(Reporter)

Updated

9 years ago
Flags: blocking1.9.1?
(Reporter)

Comment 9

9 years ago
Ping? Could this patch please get applied? Mark agrees that it is needed for gecko 1.9.1:-)
Attachment #380068 - Flags: approval1.9.1? → approval1.9.1+
You have my approval to land it on mozilla-central. If you don't break any tests there, go ahead and land it on mozila-191.

Also: in the future you can run this through tryserver to see if it breaks tests without touching mozilla-central. Also-also: in the future, you should be including tests of your own so we don't break this.
(also, I think this is NPOTB and doesn't need approval, but thanks for checking!)
Flags: blocking1.9.1? → blocking1.9.1-

Comment 12

9 years ago
beltzner: tryserver would require an hg account, the people asking for their work to be committed in this bug (excluding myself) do not have one (hopefully we can get that changed at some point...).

but thanks for explaining NPOTB, hopefully this will make people like mw22 less likely to refuse requests :).
Pushed http://hg.mozilla.org/mozilla-central/rev/fdecaf027ede
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.