Closed Bug 1138788 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: RyanVM, Assigned: heycam)

Details

(Keywords: assertion)

Attachments

(1 file, 1 obsolete file)

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.
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.
Attached patch patch (obsolete) — Splinter Review
Creating a null principal seems the easiest thing to do.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8571757 - Flags: review?(dholbert)
Comment on attachment 8571757 [details] [diff] [review]
patch

Seems reasonable.
Attachment #8571757 - Flags: review?(dholbert) → review+
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....
Flags: needinfo?(cam)
(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.
Flags: needinfo?(cam)
Attached patch patch v2Splinter Review
Attachment #8571757 - Attachment is obsolete: true
Attachment #8572451 - Flags: review?(dholbert)
Attachment #8572451 - Attachment is patch: true
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/
Attachment #8572451 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/93e76e5a8186
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.