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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: ferjm, Assigned: ferjm)

Tracking

(Blocks: 1 bug, {crash})

unspecified
mozilla55
crash
Points:
---

Firefox Tracking Flags

(firefox53 unaffected, firefox54 unaffected, firefox55 fixed, firefox-esr52 unaffected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
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
(Assignee)

Updated

2 months ago
Assignee: nobody → ferjmoreno
Priority: -- → P2
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 hidden (mozreview-request)

Comment 3

2 months ago
mozreview-review
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?

Updated

2 months ago
Flags: needinfo?(bbirtles)
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.
Flags: needinfo?(bbirtles)
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

2 months ago
mozreview-review
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.
Attachment #8869395 - Flags: review?(hikezoe)
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.
Flags: needinfo?(bbirtles)
Yeah, I don't think there's any need to preserve the original shorthand value for (simulated) failed values.
Flags: needinfo?(bbirtles)
Comment hidden (mozreview-request)

Comment 10

2 months ago
mozreview-review
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.
Attachment #8869395 - Flags: review?(hikezoe) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Keywords: crash

Comment 12

2 months ago
Pushed by ferjmoreno@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ab591336a1a3
stylo: Simulate compute value failure for dom/animation mochitests. r=hiro

Comment 13

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ab591336a1a3
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thank you Fernando!
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
You need to log in before you can comment on or make changes to this bug.