Startup crash in v41 nsStyleSet::GatherRuleProcessors(nsStyleSet::sheetType) possibly related to Yandex toolbar *and* Adblock Plus

RESOLVED FIXED in Firefox 41

Status

()

Core
CSS Parsing and Computation
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dmajor, Assigned: heycam)

Tracking

({crash})

41 Branch
mozilla44
x86
Windows NT
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed, firefox43 fixed, firefox44 fixed, relnote-firefox 41+)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-1a47824e-7d37-494e-bb22-c92b92150923.
=============================================================

This is #4 on the topcrash scores on 41.0 mostly due to being a startup crash. It is highly correlated with "ru" locale setting. My initial instinct is that it's probably due to Yandex toolbar, but I can't confirm right now since correlation files are currently broken.

0 	xul.dll 	nsStyleSet::GatherRuleProcessors(nsStyleSet::sheetType) 	layout/style/nsStyleSet.cpp
1 	xul.dll 	nsStyleSet::EndUpdate() 	layout/style/nsStyleSet.cpp
2 	xul.dll 	PresShell::AddUserSheet(nsISupports*) 	layout/base/nsPresShell.cpp
3 	xul.dll 	PresShell::Observe(nsISupports*, char const*, wchar_t const*) 	layout/base/nsPresShell.cpp
4 	xul.dll 	nsObserverList::NotifyObservers(nsISupports*, char const*, wchar_t const*) 	xpcom/ds/nsObserverList.cpp
5 	xul.dll 	nsObserverService::NotifyObservers(nsISupports*, char const*, wchar_t const*) 	xpcom/ds/nsObserverService.cpp
6 	xul.dll 	nsStyleSheetService::LoadAndRegisterSheet(nsIURI*, unsigned int) 	layout/base/nsStyleSheetService.cpp
7 	xul.dll 	NS_InvokeByIndex 	xpcom/reflect/xptcall/md/win32/xptcinvoke.cpp
8 	xul.dll 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	js/xpconnect/src/XPCWrappedNative.cpp
9 	xul.dll 	XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp
10 	xul.dll 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
(Reporter)

Comment 1

3 years ago
On each of the aurora, beta, and release channels, this definitely looks to have been introduced on the 41 train (insufficient data for nightly).
(Reporter)

Comment 2

3 years ago
http://hg.mozilla.org/releases/mozilla-release/annotate/78c82e5cd777/layout/style/nsStyleSet.cpp#l526

PresContext() crashes because mRuleTree is null.

dbaron - is this something that a toolbar would be able to cause? Has anything changed along this codepath in version 41?
Flags: needinfo?(dbaron)
(Reporter)

Comment 3

3 years ago
I spot-checked a dozen reports and they all have in common these extensions:
 	yasearch@yandex.ru
 	{d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d} (Adblock Plus)
So mRuleTree being null means that nsStyleSet::Shutdown got called, I believe.  Or, I guess mRuleTree::Init never got called...

This is called from PresShell::Destroy.  The ordering there is mStyleSet->BeginShutdown(), some other cleanup, mStyleSet->Shutdown().

The stack above shows JS code from Adblock Plus, presumably, running and adding a user sheet, which sends an observer notification, which the presshell is responding to.  Presshell registers for these observer notifications in PresShell::Init and removes itself from observing them in PresShell::Destroy, before shutting down the styleset.

Which is to say I'm not quite sure how we can end up with a presshell that has a shutdown style set but is still getting the observer notification.  :(

That said, there are some bandaids we could apply here.

1)  nsStyleSet::GatherRuleProcessors could bail out if mInShutdown.  This should be
    enough to stop the crash if we actually had nsStyleSet::Shutdown called on us.
2)  We could check for mIsDestroying in PresShell::Observe and bail out if so.  That would
    fix things if our presshell is somehow receiving observer notifications even though
    it's RemoveObserver'd itself.

Does the "yasearch@yandex.ru" addon include binary blobs?  If not, it might be interesting to see what it's actually doing to cause this...
(In reply to David Major [:dmajor] from comment #2)
> http://hg.mozilla.org/releases/mozilla-release/annotate/78c82e5cd777/layout/
> style/nsStyleSet.cpp#l526
> 
> PresContext() crashes because mRuleTree is null.
> 
> dbaron - is this something that a toolbar would be able to cause? Has
> anything changed along this codepath in version 41?

The big thing that changed in 41 in this area is bug 77999.  I don't have any ideas immediately, but maybe heycam does.
Flags: needinfo?(dbaron) → needinfo?(cam)
Summary: Startup crash in v41 nsStyleSet::GatherRuleProcessors(nsStyleSet::sheetType) possibly related to Yandex toolbar → Startup crash in v41 nsStyleSet::GatherRuleProcessors(nsStyleSet::sheetType) possibly related to Yandex toolbar *and* Adblock Plus
Note that the crashing code was entirely added in bug 77999.  Until then, this function didn't use to call PresContext().
(Reporter)

Comment 7

3 years ago
The correlations files are working again: (I removed the 0% lines)

    100% (84/84) vs.   3% (558/19077) yasearch@yandex.ru (Yandex.Bar, https://addons.mozilla.org/addon/3495)
          1% (1/84) vs.   0% (1/19077) 8.13.0
          6% (5/84) vs.   0% (17/19077) 8.13.1
         76% (64/84) vs.   2% (425/19077) 8.13.5.2
          4% (3/84) vs.   0% (5/19077) 8.8.1
         13% (11/84) vs.   0% (40/19077) 8.9.2

     90% (76/84) vs.   3% (609/19077) vb@yandex.ru
          4% (3/84) vs.   0% (14/19077) 2.17.0
         12% (10/84) vs.   0% (48/19077) 2.18.0
         73% (61/84) vs.   2% (445/19077) 2.22.1
          2% (2/84) vs.   0% (33/19077) 2.22.5.2

     98% (82/84) vs.  20% (3872/19077) {d10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d} (Adblock Plus, https://addons.mozilla.org/addon/1865)
          2% (2/84) vs.   1% (98/19077) 2.6.10
         95% (80/84) vs.  20% (3734/19077) 2.6.11
(Assignee)

Comment 8

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #6)
> Note that the crashing code was entirely added in bug 77999.  Until then,
> this function didn't use to call PresContext().

Yeah, it looks like GatherRuleProcessors could have happily done its work even when mRuleTree is null, before bug 77999.

If we're in here after style set shutdown, then bailing out seems like a reasonable band-aid.  If we haven't called nsStyleSet::Init yet then bailing out early is probably OK -- we'll surely end up calling GatherRuleProcessors soon anyway.

At the top of the stack is a Promise callback, which makes me think that it's more likely we're being being called after style set shutdown.

There are plenty of NS_ENSURE_FALSE(mInShutdown, ...) calls at the top of methods in nsStyleSet, incidentally.
(Assignee)

Comment 9

3 years ago
I tried installing Yandex.Bar 8.13.5.2 and ABP 2.6.11 but couldn't reproduce the crash.
(Assignee)

Comment 10

3 years ago
Both Yandex.Bar and ABP call loadAndRegisterSheet(..., USER_SHEET), and Yandex.Bar uses promises a bit, although I couldn't find an obvious .then(... loadAndRegisterSheet(..., USER_SHEET) ...) call.

I lean towards adding an NS_ENSURE_FALSE(mInShutdown, NS_ERROR_FAILURE) at the top of GatherRuleProcessors.
Flags: needinfo?(cam)
(Assignee)

Comment 11

3 years ago
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #10)
> I lean towards adding an NS_ENSURE_FALSE(mInShutdown, NS_ERROR_FAILURE) at
> the top of GatherRuleProcessors.

The only thing returning NS_ERROR_FAILURE here would affect is making nsStyleSet::EndUpdate break out of its loop.  Nothing seems to check nsStyleSet::EndUpdate's return value.
Sounds reasonable.  I don't quite see how this is happening, but it's worth seeing if it makes the crash go away on aurora and beta.
(Assignee)

Comment 13

3 years ago
(In reply to Cameron McCormack (:heycam) (away Oct 2) from comment #8)
> At the top of the stack is a Promise callback, which makes me think that
> it's more likely we're being being called after style set shutdown.

It's also less likely we're called before nsStyleSet::Init, since we call nsStyleSet::Init just before we set nsPresShell::mStyleSet.
(Assignee)

Comment 14

3 years ago
(In reply to Boris Zbarsky [:bz] from comment #4)
> 2)  We could check for mIsDestroying in PresShell::Observe and bail out if
> so.  That would
>     fix things if our presshell is somehow receiving observer notifications
> even though
>     it's RemoveObserver'd itself.

I think this is reasonable to do as well.  Let's do it.

As an aside, in PresShell::Observe we check for mStyleSet not being null -- i.e. checking we're not called before the pres shell has had its Init called -- although we do set mStyleSet before we register the observer so I don't think we'll ever have mStyleSet being null in there.
(Assignee)

Comment 15

3 years ago
Created attachment 8667071 [details] [diff] [review]
patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=72f23da8beec
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8667071 - Flags: review?(dbaron)
Attachment #8667071 - Flags: review?(dbaron) → review+
https://hg.mozilla.org/mozilla-central/rev/d22d047dd978
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Reporter)

Comment 18

3 years ago
dbaron, to save some time, would you mind filling out the uplift on behalf of heycam?
Flags: needinfo?(dbaron)
Comment on attachment 8667071 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 77999
[User impact if declined]: probably continue crashing with combination of yandex toolbar and adblock plus
[Describe test coverage new/current, TreeHerder]: none (can't reproduce crash, either)
[Risks and why]: low risk; skips some work when document already partly destroyed
[String/UUID change made/needed]: no
Flags: needinfo?(dbaron)
Attachment #8667071 - Flags: approval-mozilla-beta?
Attachment #8667071 - Flags: approval-mozilla-aurora?
David, do you believe this patch is safe to be uplifted to mozilla-release for inclusion in 41.0.1 release? If yes, could you please nominate for uplift to m-r? DMajor brought it to my attention again today and suggested we take it in 41.
Flags: needinfo?(dbaron)
If we're going to take it in 41.0.1, I might want to take only the nsStyleSet.cpp half of the patch there -- at least depending on the schedule for 41.0.1.  (It would probably be good to get confirmation that the fix works as well, but sounds like there's not time for that.)
Flags: needinfo?(dbaron)
Comment on attachment 8667071 [details] [diff] [review]
patch

Approval Request Comment:  see comment 19

For release, I'd like to land the nsStyleSet.cpp patch only.
Attachment #8667071 - Flags: approval-mozilla-release?
Curious if you agree if that's a reasonable strategy.
Flags: needinfo?(bzbarsky)
That seems fine, yes.
Flags: needinfo?(bzbarsky)
Comment on attachment 8667071 [details] [diff] [review]
patch

Approved for aurora uplift.
Comment on attachment 8667071 [details] [diff] [review]
patch

Given that AdBlock plus is one of our top addons, fixing crashes in that area meets the 41 dot release criteria bar. Approving for uplift to m-r.
Attachment #8667071 - Flags: approval-mozilla-release? → approval-mozilla-release+

Updated

3 years ago
status-firefox41: --- → affected
Comment on attachment 8667071 [details] [diff] [review]
patch

Turning the right flag on, please see Liz's comment 25.
Attachment #8667071 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox42: --- → affected
status-firefox43: --- → affected
We tried to reproduce this initial crash on various OS`s (Windows 10 32-bit, Windows 7 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit) with Firefox 41 RC (affected) using Yandex toolbar and Adblock Plus but with no luck. 

Also did not reproduce with latest Firefox 41.0.1 build 2 on the same OS`s. 
I'm not going to mark the flag as verified until Socorro does not show any more crashes on latest builds.
Comment on attachment 8667071 [details] [diff] [review]
patch

Taking it to beta too.
Attachment #8667071 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Added to the release notes with "Fix a startup crash related to Yandex toolbar and Adblock Plus (1209124)" as wording.
relnote-firefox: --- → 41+
You need to log in before you can comment on or make changes to this bug.