1433056 - Tab crashes when performing automatically selections using Firefox Screenshots

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cristina Badescu [:cbadescu], Experiments QA]

Attachment: Tab Crashes - Screenshots.gif

[Notes]:
- This issue is not reproducible for manual selections.

[Regression]:
- I've managed to find a regression window using Mozregression tool. Here are the results:
Last good revision: b529d05259c5f20af811903842655c5061ffb951
First bad revision: 60d339bdf87efaecb07bb108326d0c70a513aa4b
Pushlog: https://goo.gl/wxngyp
From the pushlog it seems that bug 1431474 has caused this. Emilio, can you please take a look at this issue?

[Affected versions]:
- Nightly 60.0a1
- Screenshots 25.0.0

[Affected Platforms]:
- All Windows
- All Mac OS
- All Linux

[Steps to reproduce]:
1. Open Nightly browser and navigate to a website.
2. Click the "Page actions" button and select "Take a Screenshot".
3. Perform an automatically selection.
4. Observe the behavior.

[Expected result]:
- The selection is performed.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Thanks for the STR! I can repro this, thank you so much.

If this is a very frequent crash I can downgrade the assertion to a debug assertion for now.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1433056: Account for pres shell destruction.

The underlying issue here is that the pres shell was already being torn down
when we flush.

The less risky change here is to just change the assertions instead of
IsSafeToFlush too, but the !isSafeToFlush || mViewManager assertion was also
firing already, because we null out the view manager from Destroy.

So this change is effectively idempotent behavior-wise (we tighten a bit the
view manager assertion, but that should be fine).

Just let me know if you want me to just loosen the assertions, but I think the
IsSafeToFlush change is nicer too.

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

Comment 4

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee63620c37eb14df4222b5360b30c7dfc24c798d

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8945474 [details]
Bug 1433056: Account for pres shell destruction.

https://reviewboard.mozilla.org/r/215626/#review221318

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89401c18ca7d
Account for pres shell destruction. r=smaug

Comment 7

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/89401c18ca7d

Comment 8

[Cristina Badescu [:cbadescu], Experiments QA]

Thanks for fixing this issue! I've retested it and is no longer reproducible on latest Nightly build 60.0a1 (2018-01-26), on Windows 10 x64, Mac 10.12.6 and Arch Linux 4.12.

Comment 9

[Calixte Denizet (:calixte)]

Crashes numbers decreased significantly since the patch landed, but there still are existing in nightly 60:
buildid 20180126035135: 14 crashes (12 installations)
buildid 20180126104626: 17 crashes (14 installations)
:emilio, should I open a new bug for them or reopen this one ?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Let's file another one, those are different assertions that I'm interested in looking into, thanks!