1367372 - crash with DOMContentLoaded method and alert + Inspect Element active

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Jose María Acuña]

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

1- go: data:text/html,<script>function loaded(){alert();}window.addEventListener('DOMContentLoaded',loaded,false);</script>
2- open Inspect Element 
3- F5
4- crash

Comment 1

[Jose María Acuña]

Firefox 53.0.3 Crash Report:

ID: be414e5e-34bf-452e-9aeb-4d33b1170524
Signature: nsDependentAtomString::nsDependentAtomString

Comment 2

[Loic]

CR FF55:
https://crash-stats.mozilla.com/report/index/c42e355f-478e-4fa6-903c-ff0c30170524

Comment 3

[Jose María Acuña]

(More information)

With the last parameter set to true, no crash occurs:

window.addEventListener('DOMContentLoaded',loaded,true);

Comment 4

[Loic]

It's a regression in FF53, I'm bisecting.

Comment 5

[Loic]

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=cbd4fb9f368e4ea3dd2efff323fc860aa407bdbe&tochange=fb4f5c7e082a4176cd1b8f3c784e5f424417e3fa

Bug 1151909 - Make the inspector actor wait for DOMContentLoaded instead of load. r=pbro

Comment 6

[Gerry Chang [:gchang]]

Track 54+/55+ as regression.

Comment 7

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

Too late to fix in 53. There are about 50 crashes with this signature on release in the last week so it isn't high volume.

Comment 8

[Patrick Brosset <:pbro>]

Reproduced with FF54, the full browser crashes as well. One thing is that you don't even need to switch to "inspect element" mode, only having the inspector opened is enough.

Alex is on PTO for a while. Forwarding to Matteo for investigation.

Comment 9

[Gerry Chang [:gchang]]

Too late for 54 and the volume of crashes for 54 is low. Mark 54 won't fix.

Comment 10

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Bug 1367372 - use capture phase in DOMContentLoaded;

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

Comment 11

[Matteo Ferretti [:zer0] [:matteo]]

Using capture phase instead of bubbling seems to fix the crash – at least on OS X; it's definitely needs more qa tests.

Comment 12

[Patrick Brosset <:pbro>]

Thank you Matteo.
As discussed on IRC, we don't exactly know why this fix works, which makes me a little uncomfortable R+'ing this.
Also, I just found out that even with this fix, the following test page crashes: 

data:text/html,<script>function loaded(){alert();}window.addEventListener('DOMContentLoaded',loaded,true);</script>

Olli: does the crash report ring any bell? https://crash-stats.mozilla.com/report/index/c42e355f-478e-4fa6-903c-ff0c30170524
Any help you could give to help us understand the issue and finding a fix would be awesome!

Comment 13

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

If I read this right, this needs use of getListenerInfo inside once: true listener.

Comment 14

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

Attachment: elm_devtools.diff

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8877168 [details] [diff] [review]
elm_devtools.diff

Review of attachment 8877168 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable. Probably also add a test?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee77dab0626a
skip to be removed listeners when asking for listener info, r=xidorn

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ee77dab0626a

Comment 18

[Loic]

Can we make obsolete the 1st patch provided by Matteo?

Comment 19

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

FWIW, I think devtools does have some bug there, if it is listening events in such way that calling stopPropagation in web pages may disturb it. But sure, that isn't about the crash.

Comment 20

[Patrick Brosset <:pbro>]

(In reply to Olli Pettay [:smaug] from comment #19)
> FWIW, I think devtools does have some bug there, if it is listening events
> in such way that calling stopPropagation in web pages may disturb it. But
> sure, that isn't about the crash.
Matteo, do you mind investigating this part, and filing another bug for that please?

Comment 21

[Matteo Ferretti [:zer0] [:matteo]]

Maybe related to bug 1177346? Definitely calling `stopPropagation` or `preventDefault` could actually impact the behaviors of highlighters, since it's content (even if anonymous), but that should be probably addressed on platform side too.

Olli, what scenario you were talking about, specifically…?

Comment 22

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

If devtools add normal event listeners (not system group listeners) especially to capture phase, any listener in web page can just call .stopPropagation() and devtools won't get the event.
I assume devtools would be broken with such pages.
Nothing to do with preventDefault().

Comment 23

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

Whatever this is doing to fix this low volume crash, seems best to keep it in 56.

Comment 24

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

Comment on attachment 8877168 [details] [diff] [review]
elm_devtools.diff

Approval Request Comment
[Feature/Bug causing the regression]: bug 1287706 
[User impact if declined]: Crashes with devtools
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: NA
[Is the change risky?]: Should be safe, effectively a null check
[Why is the change risky/not risky?]: see above
[String changes made/needed]: NA

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8877168 [details] [diff] [review]
elm_devtools.diff

prevent a crash with devtools, beta55+

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/362e0eeb5919

Comment 27

[Andrei Vaida [:avaida]]

(In reply to Olli Pettay [:smaug] from comment #24)
> [Is this code covered by automated tests?]: no
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no 

Setting qe-verify- based on smaug's assesment on manual testing needs.

Comment 28

[Saheda Reza [:Antora]]

I have reproduced this bug according to (2017-05-24)

Fixing bug is verified on Latest Beta--
Build ID   : 20170710085521
User Agent : Mozilla/5.0 (Windows NT 6.1; rv:55.0) Gecko/20100101 Firefox/55.0

Tested OS-- Windows7 32bit