1138788 - Lots of "Codepaths that expect to parse URLs MUST pass in an origin principal" assertion spam during mochitest-dt runs

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Going through the log below:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-debug/1425329849/mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm115-tests1-linux64-build7.txt.gz

I'm seeing the same assertion repeated many times throughout the run:
14:15:20     INFO -  [2364] ###!!! ASSERTION: Codepaths that expect to parse URLs MUST pass in an origin principal: 'Not Reached', file /builds/slave/m-cen-l64-d-000000000000000000/build/src/layout/style/nsCSSParser.cpp, line 7602
14:15:20     INFO -  #01: CSSParserImpl::ParseVariant [layout/style/nsCSSParser.cpp:7276]
14:15:20     INFO -  #02: CSSParserImpl::ParseSingleValueProperty [layout/style/nsCSSParser.cpp:10137]
14:15:20     INFO -  #03: CSSParserImpl::ParseBackgroundItem [layout/style/nsCSSParser.cpp:10533]
14:15:20     INFO -  #04: CSSParserImpl::ParsePropertyByFunction [layout/style/nsCSSParser.cpp:10334]
14:15:20     INFO -  #05: CSSParserImpl::ParseProperty [layout/style/nsCSSParser.cpp:9763]
14:15:20     INFO -  #06: CSSParserImpl::IsValueValidForProperty [layout/style/nsCSSParser.cpp:15284]
14:15:20     INFO -  #07: inDOMUtils::CssPropertyIsValid(nsAString_internal const&, nsAString_internal const&, bool*) [layout/inspector/inDOMUtils.cpp:881]
14:15:20     INFO -  #08: NS_InvokeByIndex [xpcom/reflect/xptcall/md/unix/xptcinvoke_x86_64_unix.cpp:176]
14:15:20     INFO -  #09: CallMethodHelper::Call() [js/xpconnect/src/xpcprivate.h:881]
14:15:20     INFO -  #10: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [js/xpconnect/src/XPCWrappedNative.cpp:1417]
14:15:20     INFO -  #11: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) [js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1144]
14:15:20     INFO -  #12: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [js/src/jscntxtinlines.h:227]
14:15:20     INFO -  #13: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:498]
14:15:20     INFO -  #14: Interpret [js/src/vm/Interpreter.cpp:2599]
14:15:20     INFO -  #15: js::RunScript(JSContext*, js::RunState&) [js/src/vm/Interpreter.cpp:448]
14:15:20     INFO -  #16: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [js/src/vm/Interpreter.cpp:519]
14:15:20     INFO -  #17: js::InvokeConstructor(JSContext*, JS::CallArgs) [js/src/vm/Interpreter.cpp:581]
14:15:20     INFO -  #18: js::InvokeConstructor(JSContext*, JS::Value, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) [js/src/vm/Interpreter.cpp:607]
14:15:20     INFO -  #19: js::jit::DoCallFallback [js/src/jit/BaselineIC.cpp:9628]
14:15:20     INFO -  #20: ??? (???:???)

This assertion appears to go back 8 years and I have no idea how long it's been in the logs (yay not caring about mochitest-bc/dt asserts), but at best it's hurting the SnR of the logs, and at worst it's exposing a real problem.

Comment 1

[Cameron McCormack (:heycam)]

At that call site, we don't actually check the return value of SetValueToURL, so luckily we end up returning true anyway.  It seems overkill to force the caller of inDOMUtils::IsValueValidForProperty to pass in a sheet principal when all we're doing is checking whether the value can be parsed correctly.

The assertion should stay, although in this case we're in no danger.

We could either create a new principal in CSSParserImpl::IsValueValidForProperty just to pass in to InitScanner, or set a flag on the parser while in IsValueValidForProperty to skip that assertion because we know we're safe.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch

Creating a null principal seems the easiest thing to do.

Comment 3

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c013059e28f

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8571757 [details] [diff] [review]
patch

Seems reasonable.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Creating a nsNullPrincipal is a fairly expensive operation, last I checked.

Why not change IsValueValidForProperty to take a principal argument and have inDOMUtils::CssPropertyIsValid pass in the subject principal?  Or do we think that using a system principal here will somehow give incorrect results?

The other option is to in fact add a way to silence the assertion (via an additional argument?) if the caller guarantees that the css::URLValue won't ever be used for anything and hence its principal doesn't matter....

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Not doing reviews right now from comment #5)
> Creating a nsNullPrincipal is a fairly expensive operation, last I checked.

OK.

> Why not change IsValueValidForProperty to take a principal argument and have
> inDOMUtils::CssPropertyIsValid pass in the subject principal?  Or do we
> think that using a system principal here will somehow give incorrect results?

There's no concern that passing in any given principal will give incorrect results, as the principal is not used in this code path.  (It is only used in the nsCSSValue::operator= member function, and that is not called under IsValueValidForProperty.

> The other option is to in fact add a way to silence the assertion (via an
> additional argument?) if the caller guarantees that the css::URLValue won't
> ever be used for anything and hence its principal doesn't matter....

I'll do that then.  It seems overkill to require the inDOMUtils::CssPropertyIsValid user to pass in a principal that is not used.

Comment 7

[Cameron McCormack (:heycam)]

Attachment: patch v2

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8572451 [details] [diff] [review]
patch v2

Review of attachment 8572451 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Just 3 minor comment nits:

::: layout/style/nsCSSParser.cpp
@@ +1210,5 @@
>  
> +#ifdef DEBUG
> +  // True if any parsing of URL values requires a sheet principal to have
> +  // been passed in the nsCSSScanner constructor.  This is usually the case.
> +  // If can be set to false, for example, when we create an nsCSSParser solely

s/If can/It can/

@@ +15294,5 @@
> +#ifdef DEBUG
> +  // We normally would need to pass in a sheet principal to InitScanner,
> +  // because we might parse a URL value.  However, we will never use the
> +  // parsed nsCSSValue (and so whether we have a sheet principal or not
> +  // doesn't really matter), so to avoid avoid the assertions in

s/avoid/avoid failing/ ?

("avoid" suggests that we skip over the assertion somehow)

Also: s/assertions/assertion/

Comment 9

[Cameron McCormack (:heycam)]

Thanks.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=93e76e5a8186

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93e76e5a8186