Closed Bug 1349187 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: General, defect)

All
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: jaws, Assigned: kats)

References

Details

(Keywords: regression)

Attachments

(1 file)

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)
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: nobody → bugmail
Attachment #8849557 - Flags: review?(enndeakin)
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
OS: Unspecified → Windows
Hardware: Unspecified → All
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+
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
https://hg.mozilla.org/mozilla-central/rev/5064bfb98923
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
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)
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 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-
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.

Attachment

General

Created:
Updated:
Size: