Firefox crashes when popups close, if 'disable popup auto hide' is enabled

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
General
--
critical
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: jaws, Assigned: kats)

Tracking

({regression})

unspecified
mozilla55
All
Windows
regression
Points:
---

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 wontfix, firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

STR:
Open the Web Console (Ctrl+Shift+K)
Click the Settings button
Enable "Enable browser chrome and add-on debugging toolboxes"
Enable "Enable remote debugging"
Open the Browser Toolbox (Ctrl+Alt+Shift+I)
Enable the "Disable popup auto hide" button
Open the following page:    data:text/html,<select><option>1<option>2
Open the select popup
Click on the webpgae

Expected:
The popup should close

Actual:
Firefox exits

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8dd496fd015a2b6e99573070279d9d1593836ea9&tochange=0cc9dced786cf2a3baeff707b636b4ad02637df0

Of that regression range, my best guess is Bug 1343977. Flagging needinfo for Kats and Neil.
Flags: needinfo?(enndeakin)
Flags: needinfo?(bugmail)

Comment 1

9 months ago
Looks like lastRollup isn't being initialized to null before calling Rollup in nsWindow::DealWithPopups. (The old code asserted when it wasn't null)

Better would be to set it to null in the Rollup implementations, even when false is returned.
Flags: needinfo?(enndeakin)
Makes sense, thanks for looking into it! I can write the patch, unless you're already doing it.
Flags: needinfo?(bugmail)
(Assignee)

Updated

9 months ago
Assignee: nobody → bugmail
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8849557 - Flags: review?(enndeakin)
(Assignee)

Updated

9 months ago
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox52: --- → unaffected
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
Keywords: regression
OS: Unspecified → Windows
Hardware: Unspecified → All

Comment 4

9 months ago
mozreview-review
Comment on attachment 8849557 [details]
Bug 1349187 - Ensure the Rollup implementations clear the out-pointer even upon returning false.

https://reviewboard.mozilla.org/r/122334/#review124444
Attachment #8849557 - Flags: review?(enndeakin) → review+

Comment 5

9 months ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5064bfb98923
Ensure the Rollup implementations clear the out-pointer even upon returning false. r=enndeakin+6102

Comment 6

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5064bfb98923
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox-esr52: --- → unaffected
Comment on attachment 8849557 [details]
Bug 1349187 - Ensure the Rollup implementations clear the out-pointer even upon returning false.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1343977
[User impact if declined]: Crash when following the STR in comment 0 (and probably other STR as well)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes, STR in comment 0
[List of other uplifts needed for the feature/fix]: bug 1343977
[Is the change risky?]: no
[Why is the change risky/not risky?]: small fix to ensure a variable is not left uninitialized.
[String changes made/needed]: none
Attachment #8849557 - Flags: approval-mozilla-beta?
Attachment #8849557 - Flags: approval-mozilla-aurora?

Comment 8

9 months ago
Hi :kats,
I have a few questions. 
1. the bug number in [Feature/Bug causing the regression] is incorrect because you just fixed the bug in 55. So it's not a feature/regression in previous versions.
2, You also marked 53/54 unaffected. I'm not sure why you nominate the uplift request for 53/54.
Flags: needinfo?(bugmail)
I requested uplift to 53/54 on the regressing bug as well. This needs to be uplifted along with that. Right now this bug does not affect 53/54 so it would be incorrect to set the flags to affected... But once bug 1343977 is uplifted those branches will be affected.
Flags: needinfo?(bugmail)

Updated

9 months ago
status-firefox53: unaffected → affected
status-firefox54: unaffected → affected
I couldn't reproduce the initial issue on Nightly 55.0a1 2017-03-16 build on Win 10 64-bit machine, Microsoft surface Pro 2 tablet with Win 10 Insider Preview build and Mac OS X 10.12.4.

Note that the option "Disable popup auto hide" is checked by default on Browser Toolbox - Settings - Available Toolbox Buttons section.

Any thoughts? Thank you!
Comment on attachment 8849557 [details]
Bug 1349187 - Ensure the Rollup implementations clear the out-pointer even upon returning false.

This patch needs to be uplift along with bug 1343977. Aurora54+.
Attachment #8849557 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 12

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0d45f6bfb7a6
status-firefox54: affected → fixed
Comment on attachment 8849557 [details]
Bug 1349187 - Ensure the Rollup implementations clear the out-pointer even upon returning false.

A little late in beta for taking this, but good that we fix it for 54.
Attachment #8849557 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
status-firefox53: affected → wontfix
Hey Liz, I'd like to ask that beta-uplift be reconsidered here. The patch is very simple and fixes a reproducible crasher.
Flags: needinfo?(lhenry)
The crasher shouldn't be on beta, because the regressing bug (1343977) wasn't uplifted to beta.
Flags: needinfo?(lhenry)
You need to log in before you can comment on or make changes to this bug.