1373601 - Crash in nsINode::GetParent with Avira Browser Safety extension

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P3)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-1eb9f2ae-96ab-4350-a143-559540170616.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsINode::GetParent() 	obj-firefox/dist/include/nsINode.h:919
1 	xul.dll 	InvalidateCanvasIfNeeded 	layout/base/nsCSSFrameConstructor.cpp:8697
2 	xul.dll 	nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool, TreeMatchContext*) 	layout/base/nsCSSFrameConstructor.cpp:8183
3 	xul.dll 	nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) 	layout/base/nsCSSFrameConstructor.cpp:7722
4 	xul.dll 	nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, bool, nsCSSFrameConstructor::RemoveFlags, nsIContent**) 	layout/base/nsCSSFrameConstructor.cpp:9829
5 	xul.dll 	mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) 	layout/base/RestyleManager.cpp:1516
6 	xul.dll 	mozilla::GeckoRestyleManager::ComputeAndProcessStyleChange(nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) 	layout/base/GeckoRestyleManager.cpp:3486
7 	xul.dll 	nsTArray_Impl<unsigned int, nsTArrayInfallibleAllocator>::AppendElement<unsigned int, nsTArrayInfallibleAllocator>(unsigned int&&) 	obj-firefox/dist/include/nsTArray.h:2128
8 	xul.dll 	nsCSSFrameConstructor::MaybeRecreateFramesForElement(mozilla::dom::Element*) 	layout/base/nsCSSFrameConstructor.cpp:9434

crash reports with this signature are spiking up in the last couple of days:
https://crash-stats.mozilla.com/signature/?product=Firefox&signature=nsINode%3A%3AGetParent&date=>%3D2016-12-16T09%3A34%3A59.000Z#graphs

these are all from 32bit versions on windows with the Avira Browser Safety addon ([e-mail], 2.4.1.16401) installed. it's a product from a german company so de builds are unproportionally affected.

Correlations for Firefox Release
(100.0% in signature vs 00.58% overall) address = 0x18
(100.0% in signature vs 01.12% overall) Addon "Avira Browser Safety" = true
(89.19% in signature vs 08.61% overall) useragent_locale = de [100.0% vs 06.66% if process_type = null]

at this point i'm unsure if it's something in 54 that triggered this spike or an update of the addon. if it's something on our part, timing-wise it would have most likely been a patch landing between 54.0b12 and b13: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_54_0b12_RELEASE&tochange=FIREFOX_54_0b13_RELEASE

Comment 1

[Marco Castelluccio [:marco]]

For now, this is #15 top crasher in Firefox 54. Could you take a look?

Comment 2

[Mats Palmgren (inactive)]

Hmm, aEndChild is not a sibling of aStartChild?  That seems weird.
I guess we could wallpaper this with a null-check if needed to stop the crashes.
It'd be nice to figure out why this is happening though...

Comment 3

[Jet Villegas (inactive)]

I opened up the addon.xpi [1] from avira.com and found that it was last updated on June 1. I think it's more likely that a change in the addon is interacting with Firefox in a weird way (likely still our bug though.) 

Note the latest version online at avira.com is is 2.4.0.16391, not 2.4.1.16401 as listed here. I'm not sure if the vendor rolled back to an earlier version?

[1] http://package.avira.com/package/abs/firefox/abs-webext.xpi

Comment 4

[Jet Villegas (inactive)]

(In reply to Jet Villegas (:jet) from comment #3)

> Note the latest version online at avira.com is is 2.4.0.16391, not
> 2.4.1.16401 as listed here. 

Correction: the manifest inside the XPI does identify itself as 2.4.1.16401

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Hmm, aEndChild is not a sibling of aStartChild?  That seems weird.

Indeed.

Specifically, we are coming from nsCSSFrameConstructor::ContentInserted which passes aChild and aChild->GetNextSibling() respectively to ContentRangeInserted.  See https://hg.mozilla.org/releases/mozilla-release/annotate/e832ed037a3c/layout/base/nsCSSFrameConstructor.cpp#l7722 linked from the stack there.

So the most likely explanation is that something between when ContentRangeInserted was called and the line where we call InvalidateCanvasIfNeeded mutated the DOM.  That's not supposed to happen.  We do dispatch all sorts of internal notifications here, and if this extensions involves binary code it could be doing something like that.  But I thought we no longer allowed binary code in extensions?

Has anyone tried reproducing this locally so far?

Comment 6

[Marco Castelluccio [:marco]]

Interestingly a lot of reports have both Avira Browser Safety and AdBlock Plus:
(98.44% in signature vs 01.39% overall) Addon "Avira Browser Safety" = true
(95.31% in signature vs 19.53% overall) Addon "Adblock Plus" = true

Comment 7

[Gerry Chang [:gchang]]

Track 54+ as there is a spike in 54 in the last couple of days.

Comment 8

[Gerry Chang [:gchang]]

Hi Florin,
Do you think QE can help reproduce the issue?

Comment 9

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I'll add this to the list of bugs to investigate. We'll see who has bandwidth to look into this.

Keeping the needinfo until we have an answer.

Comment 10

[Gabi Cheta [:Gabi] Desktop QA]

I tried to reproduce the crash on Win 10 x32 using Fx 54.0 Build ID: 20170608105825 with "Avira Browser Safety Add-on" enabled with no luck.
Unable to reproduce the issue with multiple add-on installed including AdBlock Plus or by following the users steps from the crash report comments section.

Comment 11

[Mike Taylor [:miketaylr]]

One of comments in a crash report says:

> Cada vez que abro esta pagina web (www.pordede.es) da estos problemas. 

(Every time I open this web page I get this problem: www.pordede.es (but that site doesn't seem to exist... :/)

Comment 12

[Nathan Froyd [:froydnj]]

Marco, do we have contacts at Avira?  Have you contacted them around this problem?

Gabi, what version of the addon were you using?  We're not sure if perhaps Avira has rolled out a new version of the addon that doesn't trigger this problem.

We also have one crash in 55, so marking as such.

Comment 13

[Marco Castelluccio [:marco]]

(In reply to Nathan Froyd [:froydnj] from comment #12)
> Marco, do we have contacts at Avira?  Have you contacted them around this
> problem?

Yes, I've sent them an email last week, but they haven't replied yet.

Comment 14

[Markus Hartung]

Hi there I'm Markus from the Avira extensions team. We released version 2.4.1.16401 on June 6th.
I also tried to reproduce this issue with 54.0 (32-bit) but no luck so far. Is there more information about the context this would happen in?

Comment 15

[Gabi Cheta [:Gabi] Desktop QA]

(In reply to Nathan Froyd [:froydnj] from comment #12)
> Marco, do we have contacts at Avira?  Have you contacted them around this
> problem?
> 
> Gabi, what version of the addon were you using?  We're not sure if perhaps
> Avira has rolled out a new version of the addon that doesn't trigger this
> problem.
> 
> We also have one crash in 55, so marking as such.


Nathan, Avira Browser Safety 2.4.1.16401: https://www.avira.com/en/avira-browser-safety

Comment 16

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

Doesn't sound like we can reproduce this (yet). Philipp tried for a couple of hours (with de locale set).  If the crash rate gets much worse, we can try to investigate again.

Comment 17

[Marco Castelluccio [:marco]]

Markus, have you tried installing AdBlock Plus alongside your extension? It seems like most users that are crashing have both extensions installed.

I also noticed a lot of users are crashing on video-playing websites. Other than the "usual ones", I see a lot of vivo.sx URLs (e.g. https://vivo.sx/f353008d3c), a lot of filmpalast.to (e.g. http://filmpalast.to/movies/view/transformers-5-the-last-knight).

Comment 18

[Markus Hartung]

We did try this, we also tried two tested two extensions which redirect the same request to two different data urls, so far we haven't been able to reproduce any crashes.
I also tested specifically the pages you mentioned.

Comment 19

[[:philipp]]

the crash signature [@ nsIFrame::SetStyleContext] has a fairly striking correlation to avira & adblock and is rising with firefox 54 too.

Comment 20

[Markus Hartung]

Is where a way to determine which API calls might be causing this, is it more likely to relate to the webRequest API or more like have something to do with content scripts injecting elements or styles?

Comment 21

[Marco Castelluccio [:marco]]

Several users are mentioning opening a new tab with Ctrl+Click and containers (https://testpilot.firefox.com/experiments/containers/).

(In reply to Markus Hartung from comment #20)
> Is where a way to determine which API calls might be causing this, is it
> more likely to relate to the webRequest API or more like have something to
> do with content scripts injecting elements or styles?

I think it's more likely to have something to do with injected elements/styles.

Comment 22

[Markus Hartung]

Are there similar issues experienced with the Adguard extension?

Comment 24

[Marco Castelluccio [:marco]]

(In reply to Markus Hartung from comment #22)
> Are there similar issues experienced with the Adguard extension?

I don't see anything similar for users with the Adguard extension:
https://crash-stats.mozilla.com/search/?addons=~adguardadblocker%40adguard.com&product=Firefox&date=%3E%3D2017-07-13T22%3A03%3A00.000Z&date=%3C2017-07-20T22%3A03%3A00.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 25

[Jet Villegas (inactive)]

Marking 'fix-optional' for 57. The few crash reports I saw on Firefox 57 appear to use the Gecko (not Stylo) style system.

Comment 26

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.