1365674 - stylo: dom/animation/test/chrome/test_simulate_compute_values_failure.html crash with Assertion failure: mUnit == eCSSUnit_TokenStream (not a token stream value)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Fernando Jiménez Moreno [:ferjm]]

Assertion failure: mUnit == eCSSUnit_TokenStream (not a token stream value), at /Users/ferjm/dev/mozilla/mozilla-central/layout/style/nsCSSValue.h:795
LLVM ERROR: IO failure on output stream.
GECKO(20772) | #01: mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(JSContext*, JS::Handle<JSObject*>, mozilla::ErrorResult&) (ErrorResult.h:380, in XUL)
GECKO(20772) | #02: already_AddRefed<mozilla::dom::KeyframeEffect> mozilla::dom::KeyframeEffectReadOnly::ConstructKeyframeEffect<mozilla::dom::KeyframeEffect, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions>(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) (ErrorResult.h:380, in XUL)
GECKO(20772) | #03: mozilla::dom::KeyframeEffect::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) (KeyframeEffect.cpp:65, in XUL)
GECKO(20772) | #04: mozilla::dom::Element::Animate(mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JSContext*, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) (AlreadyAddRefed.h:124, in XUL)
GECKO(20772) | #05: mozilla::dom::Element::Animate(JSContext*, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeAnimationOptions const&, mozilla::ErrorResult&) (Maybe.h:224, in XUL)
GECKO(20772) | #06: mozilla::dom::ElementBinding::animate(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) (AlreadyAddRefed.h:146, in XUL)
GECKO(20772) | #07: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (BindingUtils.cpp:2956, in XUL)
GECKO(20772) | #08: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:294, in XUL)
GECKO(20772) | #09: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:470, in XUL)
GECKO(20772) | #10: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:521, in XUL)
GECKO(20772) | #11: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:410, in XUL)
GECKO(20772) | #12: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:488, in XUL)
GECKO(20772) | #13: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (Interpreter.cpp:534, in XUL)
GECKO(20772) | #14: js::fun_apply(JSContext*, unsigned int, JS::Value*) (jsfun.cpp:1288, in XUL)
GECKO(20772) | #15: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:294, in XUL)
GECKO(20772) | #16: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:470, in XUL)
GECKO(20772) | #17: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:521, in XUL)
GECKO(20772) | #18: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:410, in XUL)
GECKO(20772) | #19: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:488, in XUL)
GECKO(20772) | #20: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:521, in XUL)
GECKO(20772) | #21: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:410, in XUL)
GECKO(20772) | #22: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:488, in XUL)
GECKO(20772) | #23: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (Interpreter.cpp:534, in XUL)
GECKO(20772) | #24: js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const (Wrapper.cpp:166, in XUL)
GECKO(20772) | #25: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const (CrossCompartmentWrapper.cpp:353, in XUL)
GECKO(20772) | #26: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (Proxy.cpp:479, in XUL)
GECKO(20772) | #27: js::proxy_Call(JSContext*, unsigned int, JS::Value*) (RootingAPI.h:818, in XUL)
GECKO(20772) | #28: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:294, in XUL)
GECKO(20772) | #29: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:452, in XUL)
GECKO(20772) | #30: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:521, in XUL)
GECKO(20772) | #31: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:410, in XUL)
GECKO(20772) | #32: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:488, in XUL)
GECKO(20772) | #33: Interpret(JSContext*, js::RunState&) (Interpreter.cpp:521, in XUL)
GECKO(20772) | #34: js::RunScript(JSContext*, js::RunState&) (Interpreter.cpp:410, in XUL)
GECKO(20772) | #35: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:488, in XUL)
GECKO(20772) | #36: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (Interpreter.cpp:534, in XUL)
GECKO(20772) | #37: js::Wrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const (Wrapper.cpp:166, in XUL)
GECKO(20772) | #38: js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) const (CrossCompartmentWrapper.cpp:353, in XUL)
GECKO(20772) | #39: js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) (Proxy.cpp:479, in XUL)
GECKO(20772) | #40: js::proxy_Call(JSContext*, unsigned int, JS::Value*) (RootingAPI.h:818, in XUL)
GECKO(20772) | #41: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) (jscntxtinlines.h:294, in XUL)
GECKO(20772) | #42: js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) (Interpreter.cpp:452, in XUL)
GECKO(20772) | #43: js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) (Interpreter.cpp:534, in XUL)
GECKO(20772) | #44: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) (jsapi.cpp:2891, in XUL)
GECKO(20772) | #45: mozilla::dom::Function::Call(JSContext*, JS::Handle<JS::Value>, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) (FunctionBinding.cpp:36, in XUL)
GECKO(20772) | #46: void mozilla::dom::Function::Call<nsCOMPtr<nsISupports> >(nsCOMPtr<nsISupports> const&, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) (FunctionBinding.h:72, in XUL)
GECKO(20772) | #47: nsGlobalWindow::RunTimeoutHandler(mozilla::dom::Timeout*, nsIScriptContext*) (nsGlobalWindow.cpp:13110, in XUL)
GECKO(20772) | #48: mozilla::dom::TimeoutManager::RunTimeout(mozilla::dom::Timeout*) (TimeoutManager.cpp:720, in XUL)
GECKO(20772) | #49: mozilla::dom::(anonymous namespace)::TimerCallback(nsITimer*, void*) (RefPtr.h:40, in XUL)
GECKO(20772) | #50: nsTimerImpl::Fire(int) (nsTimerImpl.cpp:489, in XUL)
GECKO(20772) | #51: nsTimerEvent::Run() (TimerThread.cpp:285, in XUL)
GECKO(20772) | #52: mozilla::ThrottledEventQueue::Inner::ExecuteRunnable() (atomic:987, in XUL)
GECKO(20772) | #53: mozilla::ThrottledEventQueue::Inner::Executor::Run() (ThrottledEventQueue.cpp:75, in XUL)
GECKO(20772) | #54: nsThread::ProcessNextEvent(bool, bool*) (Maybe.h:224, in XUL)
GECKO(20772) | #55: NS_ProcessPendingEvents(nsIThread*, unsigned int) (nsThreadUtils.cpp:335, in XUL)
GECKO(20772) | #56: nsBaseAppShell::NativeEventCallback() (nsBaseAppShell.cpp:98, in XUL)
GECKO(20772) | #57: nsAppShell::ProcessGeckoEvents(void*) (nsAppShell.mm:400, in XUL)
GECKO(20772) | #58: CFStringFold (in CoreFoundation) + 4209
GECKO(20772) | #59: -[_CFXNotificationRegistrar match:object:observer:enumerator:] (in CoreFoundation) + 1373
GECKO(20772) | #60: -[NSTaggedPointerString uppercaseStringWithLocale:] (in CoreFoundation) + 166
GECKO(20772) | #61: -[__NSDictionaryI countByEnumeratingWithState:objects:count:] (in CoreFoundation) + 100
GECKO(20772) | #62: RunCurrentEventLoopInMode (in HIToolbox) + 240
GECKO(20772) | #63: ReceiveNextEventCommon (in HIToolbox) + 432
GECKO(20772) | #64: _BlockUntilNextEventMatchingListInModeWithFilter (in HIToolbox) + 71
GECKO(20772) | #65: _DPSNextEvent (in AppKit) + 1120
GECKO(20772) | #66: -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:] (in AppKit) + 2796
GECKO(20772) | #67: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] (nsAppShell.mm:130, in XUL)
GECKO(20772) | #68: -[NSApplication run] (in AppKit) + 926
GECKO(20772) | #69: nsAppShell::Run() (nsCOMPtr.h:551, in XUL)
GECKO(20772) | #70: nsAppStartup::Run() (nsAppStartup.cpp:283, in XUL)
GECKO(20772) | #71: XREMain::XRE_mainRun() (nsAppRunner.cpp:4550, in XUL)
GECKO(20772) | #72: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) (nsAppRunner.cpp:4730, in XUL)
GECKO(20772) | #73: XRE_main(int, char**, mozilla::BootstrapConfig const&) (nsAppRunner.cpp:4823, in XUL)
GECKO(20772) | #74: main (nsBrowserApp.cpp:236, in firefox)
TEST-INFO | Main app process: exit 1

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I should note about this failure.

In this test case, we are using simulateComputeValuesFailure flag (the flag is only for testing purpose), and when the flag is set, we are storing a given string for shorthand property in nsCSSValue as a token stream if the given string can't be parsed.  Unfortunately in stylo, we don't use nsCSSValue anymore, so we should store the given string somewhere else in the case where simulateComputeValuesFailure is set.

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Bug 1365674 - stylo: Simulate compute value failure for dom/animation mochitests.

Review commit: https://reviewboard.mozilla.org/r/141052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/141052/

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8869395 [details]
Bug 1365674 - stylo: Simulate compute value failure for dom/animation mochitests.

https://reviewboard.mozilla.org/r/141052/#review144778

Thanks for doing this!
I am very impressed that the patch came so quickly.

A couple of concerns:
1) The additional flag in PropertyDeclarationBlock
   PropertyDeclarationBlock is used everywhere, I don't think the additional flas is acceptable. The flag is only for testing purpose, if we have no other choise, we should enable it only for debug build (using #[cfg(debug_assertions)]?, I am not sure) and run this test only on debug build
2) The shorthand property value is not stored?
   It seems to me that this patch does not store the shorthand property value at all. It looks that it relies on missing keyframe values. I am inclined to drop simulateComputeValuesFailure flag....

Brian, what do you think?

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

What if we just add a mSimulateComputeValueFailure flag to PropertyValuePair instead and use that for both Servo and Gecko?

That would mean reworking the Gecko side too but would be less invasive that modifying PropertyValueDeclarationBlock (and probably simpler than what we currently do with token stream values). We could potentially make the member debug-only and perhaps mark the corresponding tests as debug-only too if we're worried about swelling the size of PropertyValuePair.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Thank you, Brian. PropertyValuePair looks a reasonable place to me.

Fernando, would you mind updating 8869395 to add mSimulateComputeValueFailure to PropertyValuePair and add an nsString to store the shorthand value string?  I'd say we should use the additional members only for debug build, but if it will be messy, I am OK with them for release build too.

Thank you!

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8869395 [details]
Bug 1365674 - stylo: Simulate compute value failure for dom/animation mochitests.

https://reviewboard.mozilla.org/r/141052/#review144944

Clearing review request for now.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Fernando told me that the test_simulate_compute_values_failure.html does not call KeyframeEffectReadOnly::GetKeyframes() at all.  I thought the test uses GetKeyframes() but actually it uses GetProperties() instead.  Using GetProperties() means that we don't directly use the tokenStream in the test case (actually indirectly used?).  Do we really need to store the shorthand value string?  Where do we use the token stream value in case the case where mSimulateComputeValueFailure is true?  I might be something, but couldn't find any clues.

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Yeah, I don't think there's any need to preserve the original shorthand value for (simulated) failed values.

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8869395 [details]
Bug 1365674 - stylo: Simulate compute value failure for dom/animation mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141052/diff/1-2/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8869395 [details]
Bug 1365674 - stylo: Simulate compute value failure for dom/animation mochitests.

https://reviewboard.mozilla.org/r/141052/#review147676

Great!!
Thanks for doing this1

::: servo/ports/geckolib/glue.rs:2389
(Diff revision 2)
> +    let is_shorthand =
> +        PropertyId::from_nscsspropertyid(property.mProperty).ok().map_or(false, |p| {

We can use get_property_id_from_nscsspropertyid! macro to return early in the case where the property can not be converted, but it's up to you.

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8869395 [details]
Bug 1365674 - stylo: Simulate compute value failure for dom/animation mochitests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141052/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab591336a1a3
stylo: Simulate compute value failure for dom/animation mochitests. r=hiro

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab591336a1a3

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Thank you Fernando!