Closed Bug 1367372 Opened 3 years ago Closed 3 years ago

crash with DOMContentLoaded method and alert + Inspect Element active

Categories

(DevTools :: Inspector, defect, P1, critical)

53 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox53 wontfix, firefox54+ wontfix, firefox55+ fixed, firefox56 verified)

VERIFIED FIXED
Firefox 56
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 + wontfix
firefox55 + fixed
firefox56 --- verified

People

(Reporter: jm.acuna73, Assigned: smaug)

References

()

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
Firefox 53.0.3 Crash Report:

ID: be414e5e-34bf-452e-9aeb-4d33b1170524
Signature: nsDependentAtomString::nsDependentAtomString
CR FF55:
https://crash-stats.mozilla.com/report/index/c42e355f-478e-4fa6-903c-ff0c30170524
Severity: normal → critical
Crash Signature: [@ nsDependentAtomString::nsDependentAtomString ]
Keywords: crash
(More information)

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

window.addEventListener('DOMContentLoaded',loaded,true);
It's a regression in FF53, I'm bisecting.
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
Blocks: 1151909
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Inspector
Ever confirmed: true
Flags: needinfo?(poirot.alex)
Keywords: regression, testcase
Version: 55 Branch → 53 Branch
Track 54+/55+ as regression.
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.
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.
Flags: needinfo?(poirot.alex) → needinfo?(zer0)
Priority: -- → P1
Too late for 54 and the volume of crashes for 54 is low. Mark 54 won't fix.
Using capture phase instead of bubbling seems to fix the crash – at least on OS X; it's definitely needs more qa tests.
Flags: needinfo?(zer0)
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!
Flags: needinfo?(bugs)
Blocks: 1287706
Flags: needinfo?(bugs)
If I read this right, this needs use of getListenerInfo inside once: true listener.
Attachment #8877168 - Flags: review?(xidorn+moz)
Comment on attachment 8877168 [details] [diff] [review]
elm_devtools.diff

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

Looks reasonable. Probably also add a test?
Attachment #8877168 - Flags: review?(xidorn+moz) → review+
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee77dab0626a
skip to be removed listeners when asking for listener info, r=xidorn
https://hg.mozilla.org/mozilla-central/rev/ee77dab0626a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Assignee: nobody → bugs
Can we make obsolete the 1st patch provided by Matteo?
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.
Attachment #8876619 - Attachment is obsolete: true
Attachment #8876619 - Flags: review?(pbrosset)
(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?
Flags: needinfo?(zer0)
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…?
Flags: needinfo?(zer0) → needinfo?(bugs)
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().
Flags: needinfo?(bugs)
Whatever this is doing to fix this low volume crash, seems best to keep it in 56.
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
Attachment #8877168 - Flags: approval-mozilla-beta?
Comment on attachment 8877168 [details] [diff] [review]
elm_devtools.diff

prevent a crash with devtools, beta55+
Attachment #8877168 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
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
QA Whiteboard: [bugday-20170712]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.