Closed Bug 1381282 Opened 2 years ago Closed 2 years ago

We may want a fix for bug 1197791 that doesn't cause performance problems

Categories

(Core :: Networking: HTTP, defect)

53 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: bzbarsky, Assigned: dragana)

References

Details

(Keywords: perf, Whiteboard: [necko-active])

Attachments

(1 file, 3 obsolete files)

The fix for bug 1197791 added an NS_NewURI call to every single attempt to report an error to the console, just to check whether "sourceName" (in nsScriptErrorBase::InitWithWindowID terms) might be a URI with a password.  This is quite expensive in unnecessary ways for things like CSS error reporting, where a large number of errors are commonly reported, all coming from the same source.

I think a better solution would be an nsIURI API that can be used to get a "safe" string to use for error reporting and use of this API in all the places that currently get a "sourceName".  This API would simply return the same thing as GetSpec for nsSimpleURI and the same thing as GetSensitiveInfoHiddenSpec for nsStandardURL.  I understand that this is a bit more fragile than the approach that was taken in bug 1197791, but it would help eliminate the performance overhead here.

To put some numbers to that overhead, the profile linked from bug 1381270 comment 0 for "Gecko" shows that the NS_NewURI calls from this are about 5% of the total CSS parsing time on apple.com and somehere around 0.3% of the total pageload time (several ms, on fast hardware; probably a lot more on mobile).

Dragana, Blake, would either of you have time to look into this?  Are there any plans for Necko to move to dropping username/password from URIs altogether, which would obviate the need for anything special here at all?
Flags: needinfo?(mrbkap)
Flags: needinfo?(dd.mozilla)
Another option, perhaps, would be to have a way to init an error with a known-safe string, and then the CSS error reporter would just get the string once.  This would be a much more targeted and safer fix than what I propose above.
(In reply to Boris Zbarsky [:bz] from comment #1)
> Another option, perhaps, would be to have a way to init an error with a
> known-safe string, and then the CSS error reporter would just get the string
> once.  This would be a much more targeted and safer fix than what I propose
> above.

Caching and reusing the result would be the best option IMO.
If that can't be done, we can probably optimize the method to skip parsing the entire URL by going through it looking for and @ symbol. If we don't find it before the path (first slash after the protocol) it's clear that we don't need to hide the password.
> Caching and reusing the result would be the best option IMO.

The CSS error reporter already caches and reuses the result; it could easily do so after the removal.  The problem is that every error it reports ends up redoing the check.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(dd.mozilla)
Whiteboard: [necko-active]
What about changing interface that can supply nsIURI * or a string and then nsScriptErrorBase can get GetSensitiveInfoHiddenSpec directly and we can do cache sensitiveinfoHiddenSpec in nsIURI.
Unfortunately, a number of callers have a "URI or some non-URI string".  For example, the CSS error reporter code is http://searchfox.org/mozilla-central/rev/88180977d79654af025158d8ebeb8c2aa11940eb/layout/style/ErrorReporter.cpp#206-215 and note the !mURI branch.
(In reply to Boris Zbarsky [:bz] from comment #5)
> Unfortunately, a number of callers have a "URI or some non-URI string".  For
> example, the CSS error reporter code is
> http://searchfox.org/mozilla-central/rev/
> 88180977d79654af025158d8ebeb8c2aa11940eb/layout/style/ErrorReporter.cpp#206-
> 215 and note the !mURI branch.

I was thinking of offering both:
1) InitWithWindowID(const nsAString& message, const nsAString& sourceName, nsIURI uri, const nsAString& sourceLine,...
where sourceName or uri is empty/null
or
2) InitWithWindowID(const nsAString& message, const nsAString& sourceName, const nsAString& sourceLine,...
and InitWithWindowID(const nsAString& message, nsIURI uri, const nsAString& sourceLine,...
and call them as needed.
That might work, but still in the CSS case it would mean we have to GetSpec every time, which is also bad (that's why the spec cache is there: GetSpec is _really_ expensive in some cases; consider long data: stylesheets).
(In reply to Boris Zbarsky [:bz] from comment #7)
> That might work, but still in the CSS case it would mean we have to GetSpec
> every time, which is also bad (that's why the spec cache is there: GetSpec
> is _really_ expensive in some cases; consider long data: stylesheets).
I can save the sensitiveinfoHiddenSpec version into spec cache.
Right, that's the proposal in comment 1.
Flags: needinfo?(mrbkap)
Attached patch bug_1381282_v1.patch (obsolete) — Splinter Review
I have change calls to InitWithWindowID to InitWithWindowIDandString and InitWithWindowIDandURI only where I was completely sure it is safe to do so. I did not have time to dig into other cases. Maybe someone alse can do that in a separate bug.

Valentin, can you look at the necko part?
Attachment #8888239 - Flags: review?(valentin.gosu)
Attachment #8888239 - Flags: review?(bzbarsky)
Comment on attachment 8888239 [details] [diff] [review]
bug_1381282_v1.patch

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

::: dom/bindings/nsIScriptError.idl
@@ +116,5 @@
> +    /* This is the same function as initWithWindowID, but it will not preform
> +     * sourceName transforming to a anonymized uri spec. Please use it olny if you
> +     * are sure anonymization is not needed!!!
> +     */
> +    [noscript] void initWithWindowIDandString(in AString message,

nit: I read this as init With Window I Dand String - maybe the A should also be capitalized?

::: netwerk/base/nsStandardURL.cpp
@@ +1389,5 @@
>  nsStandardURL::GetSensitiveInfoHiddenSpec(nsACString &result)
>  {
>      nsresult rv = GetSpec(result);
>      if (NS_FAILED(rv)) {
> +        result.Assign("[nsIURI::GetSpec failed]");

I don't think this is the right approach - for 2 reasons.
1. GetSpec might fail, due to OOM, or other reasons. On the next lines we call string.Replace, which could turn into a seg-fault if the string is shorter than expected.
2. We should actually propagate the error result if we have one.

The caller of this method can manually check the error code and use the error message.
Attachment #8888239 - Flags: review?(valentin.gosu)
(In reply to Valentin Gosu [:valentin] from comment #12)
> Comment on attachment 8888239 [details] [diff] [review]
> bug_1381282_v1.patch
> 
> Review of attachment 8888239 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bindings/nsIScriptError.idl
> @@ +116,5 @@
> > +    /* This is the same function as initWithWindowID, but it will not preform
> > +     * sourceName transforming to a anonymized uri spec. Please use it olny if you
> > +     * are sure anonymization is not needed!!!
> > +     */
> > +    [noscript] void initWithWindowIDandString(in AString message,
> 
> nit: I read this as init With Window I Dand String - maybe the A should also
> be capitalized?
> 
> ::: netwerk/base/nsStandardURL.cpp
> @@ +1389,5 @@
> >  nsStandardURL::GetSensitiveInfoHiddenSpec(nsACString &result)
> >  {
> >      nsresult rv = GetSpec(result);
> >      if (NS_FAILED(rv)) {
> > +        result.Assign("[nsIURI::GetSpec failed]");
> 
> I don't think this is the right approach - for 2 reasons.
> 1. GetSpec might fail, due to OOM, or other reasons. On the next lines we
> call string.Replace, which could turn into a seg-fault if the string is
> shorter than expected.
> 2. We should actually propagate the error result if we have one.
> 
> The caller of this method can manually check the error code and use the
> error message.

You are right, I will update the patch.
Attached patch bug_1381282_v2.patch (obsolete) — Splinter Review
Attachment #8888239 - Attachment is obsolete: true
Attachment #8888239 - Flags: review?(bzbarsky)
Attachment #8888292 - Flags: review?(valentin.gosu)
Attachment #8888292 - Flags: review?(bzbarsky)
Attachment #8888292 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8888292 [details] [diff] [review]
bug_1381282_v2.patch

I don't see why the callers of InitWithWindowIDAndString() this patch adds are safe.  Apart from the style errorreporter, where are they sanitizing the username out?

The changes to js/xpconnect/src/XPCComponents.cpp particularly don't make sense to me...

As far as general API, I don't think we need to keep mentioning windowid; it was mentioned only to distinguish from init().  For the new APIs, I'd say we should have something like:

  initWithSanitizedSource

and

  initWithSourceURI

taking a string and a URI respectively.  And is there a reason to make those methods noscript?

Also, please fix the various typos in comments...
Attachment #8888292 - Flags: review?(bzbarsky) → review-
Attached patch bug_1381282_v3.patch (obsolete) — Splinter Review
Sorry, I have over seen, the next parameter was EmptyString()....

I hould not sit on a meeting, try to follow presentations and write code... :)
Attachment #8888292 - Attachment is obsolete: true
Attachment #8888645 - Flags: review?(bzbarsky)
Comment on attachment 8888645 [details] [diff] [review]
bug_1381282_v3.patch

>+++ b/dom/bindings/nsIScriptError.idl
>+     * This function will check whether sourceName is an uri and sanitized
>+     * it if needed!!!

  This function will check whether sourceName is a uri and sanitize it if needed.
  If you know the source name is sanitized already, use initWithSanitizedSource.

>+    /* This is the same function as initWithWindowID, but it expect an already

"expects".

>+     * Please use it only if sourceName string is already sanitized!!!

Please drop the "!!!" bit; just use '.'.

It's probably a good idea to document what "sanitized" means here.  Specifically, point to sensitiveInfoHiddenSpec.

>+    [noscript] void initWithSanitizedSource(in AString message,

Again, why is this noscript?

>+++ b/dom/bindings/nsScriptError.cpp
>+AssignSourceNameHelper(nsString& aSourceNameDest, nsIURI* aSourceURI)

Normally the outparam would come last.

>+    nsCOMPtr<nsISensitiveInfoHiddenURI> safeUri = do_QueryInterface(aSourceURI);

This is probably ok for now, but as a followup, why not move sensitiveInfoHiddenSpec up to nsIURI, so we can stop having to QI here and elsewhere?

In any case, a shared helper method for this "get a sanitized uri string from a uri" bit would be good, so we don't have multiple copies of it around.

>+nsScriptErrorBase::InitWithSanitizedSource(const nsAString& message,
>+    mMessage.Assign(message);

Instead of all this copy/paste, please just have a helper that does all the assigning except mSourceName and gets called from all three InitWith* methods.

>+++ b/dom/security/FramingChecker.cpp
>+  rv = errorObject->InitWithSanitizedSource(msg, EmptyString(), EmptyString(),

Please add a comment that InitWithSanitizedSource is OK because the source string is empty.

>+++ b/layout/style/ErrorReporter.cpp
>+    // It is safe to used InitWithWindowIDAndString because mFileName is

There is no InitWithWindowIDAndString.  Please fix the comment.

r=me with the above fixed.
Attachment #8888645 - Flags: review?(bzbarsky) → review+
Attachment #8888645 - Attachment is obsolete: true
Attachment #8893788 - Flags: review+
Comment on attachment 8893788 [details] [diff] [review]
bug_1381282_v3.patch

Can you please take a look at this patck, mainly the part where I have added NS_GetSanitizedURIStringFromURI?
Thank you.
Attachment #8893788 - Flags: review?(valentin.gosu)
Comment on attachment 8893788 [details] [diff] [review]
bug_1381282_v3.patch

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

::: dom/bindings/nsScriptError.cpp
@@ +255,5 @@
>                                      const nsACString& category,
>                                      uint64_t aInnerWindowID)
>  {
> +    InitializationHelper(message, sourceLine, lineNumber, columnNumber, flags,
> +                         category, aInnerWindowID);    

nit: trailing whitespace

::: netwerk/base/nsNetUtil.cpp
@@ +1594,5 @@
> +    }
> +
> +    if (NS_SUCCEEDED(rv)) {
> +        aSanitizedSpec.Assign(NS_ConvertUTF8toUTF16(cSpec));
> +    }

From what I can tell, most callers when we return an error code, need to handle it and assign the result to [nsIURI::GetSpec failed].
We could just do that here, no?
Attachment #8893788 - Flags: review?(valentin.gosu) → review+
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44fec2ed960a
Change nsScriptErrorBase::InitWithWindowID so that it does not call GetSensitiveInfoHiddenSpec as much as now. r=bz r=valentin
https://hg.mozilla.org/mozilla-central/rev/44fec2ed960a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
This seems to have improved performance on some stylo microbenchmarks:

== Change summary for alert #8626 (as of August 07 2017 13:56 UTC) ==

Improvements:

  5%  Stylo Servo_StyleSheet_FromUTF8Bytes_Bench windows7-32 opt      83,040.92 -> 78,771.33
  4%  Stylo Gecko_nsCSSParser_ParseSheet_Bench windows7-32 opt        76,603.33 -> 73,252.92

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8626
Does this one make it into 56? (53 branch?)
(In reply to Worcester12345 from comment #26)
> Does this one make it into 56? (53 branch?)

no it is only in 57.
You need to log in before you can comment on or make changes to this bug.