1251848 - Various reftests & crashtests are going to permafail when Gecko 47 is merged to Beta [@ mozilla::StyleSheetHandle::Ptr::IsServo() const]

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Merge day perma-crashes across two test suites.

Something not happy about RELEASE_BUILD being set? I see that Cam is out for the next couple weeks, can you please take a look, Dan?

https://treeherder.mozilla.org/logviewer.html#?job_id=17312105&repo=try

 20:43:59     INFO -  Assertion failure: mValue, at /builds/slave/try-l64-d-00000000000000000000/build/src/obj-firefox/dist/include/mozilla/StyleSheetHandle.h:52
 20:43:59     INFO -  #01: mozilla::StyleSheetHandle::Ptr::IsGecko() const [layout/style/StyleSheetHandle.h:49]
 20:43:59     INFO -  #02: nsLayoutStylesheetCache::InvalidateSheet(mozilla::HandleRefPtr<mozilla::StyleSheetHandle>*, mozilla::HandleRefPtr<mozilla::StyleSheetHandle>*) [layout/style/nsLayoutStylesheetCache.cpp:772]
 20:43:59     INFO -  #03: mozilla::ValueObserver::Observe(nsISupports*, char const*, char16_t const*) [xpcom/glue/nsTArray.h:361]
 20:43:59     INFO -  #04: nsPrefBranch::NotifyObserver(char const*, void*) [xpcom/string/nsString.h:79]
 20:43:59     INFO -  #05: pref_DoCallback [modules/libpref/prefapi.cpp:916]
 20:43:59     INFO -  #06: pref_HashPref [modules/libpref/prefapi.cpp:770]
 20:43:59     INFO -  #07: PREF_SetBoolPref [modules/libpref/prefapi.cpp:265]
 20:43:59     INFO -  #08: nsPrefBranch::SetBoolPref(char const*, bool) [modules/libpref/nsPrefBranch.cpp:161]
 20:43:59     INFO -  #09: NS_InvokeByIndex [xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:176]
 20:43:59     INFO -  #10: CallMethodHelper::Call() [js/xpconnect/src/xpcprivate.h:859]
 20:43:59     INFO -  #11: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [js/xpconnect/src/XPCWrappedNative.cpp:1367]
 20:43:59     INFO -  #12: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) [js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1115]
 20:43:59     INFO -  #13: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:236]
 20:43:59     INFO -  #14: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:478]
 20:43:59     INFO -  #15: Interpret [js/src/vm/Interpreter.cpp:2802]
 20:43:59     INFO -  #16: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:428]
 20:43:59     INFO -  #17: js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:496]
 20:43:59     INFO -  #18: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:530]
 20:43:59     INFO -  #19: js::jit::DoCallFallback [js/src/jit/BaselineIC.cpp:6136]
20:43:59 INFO - #20: ??? (???:???)

Comment 2

[Daniel Holbert [:dholbert]]

The stack in comment 0 is from having a null stylesheet (the handle's mValue being null), while we evaluate this assertion:
> 765 nsLayoutStylesheetCache::InvalidateSheet(StyleSheetHandle::RefPtr* aGeckoSheet,
> 766                                          StyleSheetHandle::RefPtr* aServoSheet)
> 767 {
[...]
> 772   MOZ_ASSERT(!aGeckoSheet || (*aGeckoSheet)->IsGecko());

This assertion was added in bug 1244074 part 4.

I think we need to change this assertion to check *aGeckoSheet (to see if the handle is pointing to something or not) *before* we call IsGecko() on it.  And similar for the assertions after it.

From a quick skim, it looks like the code does correctly check for this -- the assertions are the only thing that's broken here.

Comment 3

[Daniel Holbert [:dholbert]]

The reason this happens during crashtests & only in release builds is that this crashtest has:
>  test-pref(layout.css.grid.enabled,true)
...and grid is disabled by default on release builds, and we have a pref-change hook that makes us invalidate cached UA stylesheets (via this nsLayoutStylesheetCache::InvalidateSheet method) when the grid pref changes.  I expect this function isn't called much beyond that.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 5

[Daniel Holbert [:dholbert]]

Basically the idea here is:
 - Factor out existing "aGeckoSheet && *aGeckoSheet" checks into a local bool. (& same with s/Gecko/Servo/)
 - Replace checks that only test the former condition (was a non-null pointer passed in) with a check on this bool (since they really want to be testing whether a *non-null-flavored handle was passed in*).

Comment 6

[Daniel Holbert [:dholbert]]

Question that arose in my head when poking around at this:
"Do we *really* need to check the pointer *as well as* the pointed-to handle for nullness?"

The answer is yes.
 (1) We do need the (existing) pointer null-checks, because we have callers that directly pass nullptr, e.g. this one:
http://mxr.mozilla.org/mozilla-central/source/layout/style/nsLayoutStylesheetCache.cpp?rev=76673f4686dd&mark=814-815#813

 (2) We do also need the (missing/added-in-this-patch) "null-flavored-handle" checks, because e.g. if we get two InvalidateSheet() calls bakc-to-back, then the first call will leave the passed-in handle as being null-flavored, and the second InvalidateSheet() call needs to be able to handle that.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: trivial crashtest manifest file which reproduces this on inbound

Here's a trivial crashtest manifest which reproduces this bug.

I can e.g. save this file as to layout/generic/crashtests/mine.list and run:
  ./mach crashtest layout/generic/crashtests/mine.list

This happens due to these lines which trigger this stylesheet cache invalidation code:
> 374     Preferences::RegisterCallback(&DependentPrefChanged,
> 375                                   "layout.css.grid.enabled");

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsLayoutStylesheetCache.cpp?rev=f1ee875a249c&mark=374-375#368

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87ea50b0a260

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87ea50b0a260

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Don't feel the need to track this since it's fixed.