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

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

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.

Comment 1

[Neil Deakin]

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.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Makes sense, thanks for looking into it! I can write the patch, unless you're already doing it.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1349187 - Ensure the Rollup implementations clear the out-pointer even upon returning false.

This ensures that the pointer is always either null or a valid nsIContent after
the call to Rollup returns, and avoids potentially leaving it as garbage. An
alternative approach would be to make the call sites responsible for ensuring it
is set to nullptr if the function returns false, but this seems safer.

Review commit: https://reviewboard.mozilla.org/r/122334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122334/

Comment 4

[Neil Deakin]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5064bfb98923

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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

Comment 8

[Gerry Chang [:gchang]]

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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 10

[Petruta Horea [:phorea], Desktop Test Engineering]

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 11

[Gerry Chang [:gchang]]

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+.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0d45f6bfb7a6

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Hey Liz, I'd like to ask that beta-uplift be reconsidered here. The patch is very simple and fixes a reproducible crasher.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

The crasher shouldn't be on beta, because the regressing bug (1343977) wasn't uplifted to beta.