Firefox crashes when accessing the dialogArguments property of a closed modal Dialog

RESOLVED FIXED in mozilla33

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: catalinb, Assigned: catalinb)

Tracking

({testcase})

unspecified
mozilla33
x86
Mac OS X
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
This bug refers to the deprecated modal dialog implementation.

Calling showModalDialog() with a special uri that closes the window and calls dialogArguments() will crash Firefox.

Comment 1

4 years ago
Minimal testcase attached using "Add an attachment" please.
Keywords: testcase-wanted
(In reply to Olli Pettay [:smaug] from comment #1)
> Minimal testcase attached using "Add an attachment" please.

catalin is actively working on this, so testcase and fixes incoming
Status: NEW → ASSIGNED
Keywords: testcase-wanted
Keywords: testcase
nsGlobalWindow::GetDialogArguments might end up dereferencing a null mDialogArguments, no?
(Assignee)

Comment 4

4 years ago
Created attachment 8433617 [details] [diff] [review]
Fix null dialogArguments deref.
(Assignee)

Comment 5

4 years ago
Comment on attachment 8433617 [details] [diff] [review]
Fix null dialogArguments deref.

This patch ensures that an 'undefined' value is returned when dereferencing a null mDialogProperties.
Attachment #8433617 - Flags: review?(bzbarsky)
(Assignee)

Updated

4 years ago
Summary: Firefox crashes when calling dialogArguments on a closed modal Dialog → Firefox crashes when accessing the dialogArguments property of a closed modal Dialog
(Assignee)

Updated

4 years ago
Blocks: 999034
Comment on attachment 8433617 [details] [diff] [review]
Fix null dialogArguments deref.

r=me
Attachment #8433617 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Also on Android (and probably B2G once they run there)
(Assignee)

Comment 11

4 years ago
Created attachment 8436085 [details] [diff] [review]
Removed the test from b2g, e10s and android targets.
Attachment #8433617 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
Created attachment 8436144 [details] [diff] [review]
Removed the test from b2g, e10s and android targets.
Attachment #8436085 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
Comment on attachment 8436144 [details] [diff] [review]
Removed the test from b2g, e10s and android targets.

I think the patch is ready for landing. Try results look good - there are two tests failing on Linux x64 debug oth which are not related to my patch.

https://tbpl.mozilla.org/?tree=Try&rev=b69e5063f7f6

Please have a look.
Attachment #8436144 - Flags: review?(jschoenick)
Comment on attachment 8436144 [details] [diff] [review]
Removed the test from b2g, e10s and android targets.

Test fix looks good, carrying over r=bz
Attachment #8436144 - Flags: review?(jschoenick) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1c4d397ba6e4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.