[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: ??? (???:???)

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.

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.

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*).

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.

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

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