Closed Bug 1019761 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: catalinb, Assigned: catalinb)

References

Details

(Keywords: testcase)

Attachments

(1 file, 2 obsolete files)

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.
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?
Attached patch Fix null dialogArguments deref. (obsolete) — Splinter Review
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)
Summary: Firefox crashes when calling dialogArguments on a closed modal Dialog → Firefox crashes when accessing the dialogArguments property of a closed modal Dialog
Blocks: 999034
Comment on attachment 8433617 [details] [diff] [review]
Fix null dialogArguments deref.

r=me
Attachment #8433617 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
Also on Android (and probably B2G once they run there)
Attachment #8433617 - Attachment is obsolete: true
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1c4d397ba6e4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: