Closed Bug 779310 Opened 9 years ago Closed 9 years ago

Remove string literals from slowSql reports

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: vladan, Assigned: vladan)

Details

Attachments

(1 file, 3 obsolete files)

Through bug 776469, we found that some extensions create prepared SQL statements out of strings containing unparameterized arguments. We should sanitize such slowSQL strings before reporting them to Telemetry
Attached patch Sanitize slowSQL reports (obsolete) — Splinter Review
Attached patch Sanitize slowSQL reports (obsolete) — Splinter Review
Attachment #648088 - Attachment is obsolete: true
Assignee: nobody → vdjeric
Attachment #648532 - Attachment is obsolete: true
Attachment #648538 - Flags: review?(taras.mozilla)
Comment on attachment 648538 [details] [diff] [review]
Sanitize slowSQL reports and re-enable slowSQL reporting



+bool
+TelemetryImpl::SanitizeSQL(nsACString &sql) {

SanitizeSQL needs a big fat comment describing what it's doing, states used.

I'm not sure why you are looking for double quotes, those can only identifier names in sql(i think). 

Please do not dup into a C string...which you inplacemodify to make append easy. Instead scan the original string, provide a new string for output, use nsDependentSubstring to avoid allocating substrings. That will be less messy.
C strings lead to issues like leaks(where is your NS_free?).
I think our strings are infallible, this will mean that the method no longer needs to have a return value(ie it can't fail).

+  fullSQL.AppendPrintf(" /* %s */", dbName.BeginReading());
A bit messy that it's being done in all cases, but that seems ok.
Attachment #648538 - Flags: review?(taras.mozilla) → review-
(In reply to Taras Glek (:taras) from comment #4)
> I'm not sure why you are looking for double quotes, those can only
> identifier names in sql(i think). 

Double quotes can also represent string literals. There was an exception made for compatibility: http://www.sqlite.org/lang_keywords.html
Applied review
Attachment #648538 - Attachment is obsolete: true
Did you mean to ask for review?
(In reply to Taras Glek (:taras) from comment #7)
> Did you mean to ask for review?

Yes. Should we wait until I add filtering for "IN" clauses containing a series of strings or numbers?
(In reply to Vladan Djeric (:vladan) from comment #8)
> (In reply to Taras Glek (:taras) from comment #7)
> > Did you mean to ask for review?
> 
> Yes. Should we wait until I add filtering for "IN" clauses containing a
> series of strings or numbers?

no, can do that in a followup
Attachment #651050 - Flags: review?(taras.mozilla)
Comment on attachment 651050 [details] [diff] [review]
Sanitize slowSQL reports and re-enable slowSQL reporting, v2

Maybe Nathan has something clever to say about this.
Attachment #651050 - Flags: review?(taras.mozilla)
Attachment #651050 - Flags: review?(nfroyd)
Attachment #651050 - Flags: review+
Comment on attachment 651050 [details] [diff] [review]
Sanitize slowSQL reports and re-enable slowSQL reporting, v2

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

r=me with the below changes.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +109,5 @@
>    struct StmtStats {
> +    struct {
> +      uint32_t hitCount;
> +      uint32_t totalTime;
> +    } mainThread;

I'd suggest making this struct a named struct, so you can say:

struct StmtStats {
  struct stats mainThread;
  struct stats otherThreads;
};

which then enables other pieces of the code to become nicer, like ReflectSQL, below.

@@ +131,2 @@
>    static void StoreSlowSQL(const nsACString &offender, uint32_t delay,
> +                           bool isSanitized);

I'd suggest adding:

enum SanitizedState { Sanitized, Unsanitized };

and making this function take an |enum SanitizedState| instead of a bool, with appropriate == checking.  (Been reading about how bools in APIs virtually always want to be better-named enum parameters.)

@@ +135,5 @@
> +                                   JSObject *obj);
> +  static bool ReflectOtherThreadsSQL(SlowSQLEntryType *entry, JSContext *cx,
> +                                    JSObject *obj);
> +  static bool ReflectSQL(SlowSQLEntryType *entry, bool mainThread, JSContext *cx,
> +                         JSObject *obj);

With the suggested named struct change, this function can just take a struct stats* pointer, appropriately selected by its caller.  No more mainThread parameter.  The checking for ->hitCount == 0 can happen here, in one place, too.
Attachment #651050 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/a8d115b3b7e1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Should we uplift this patch to re-enable slowSQL reporting? Maybe after it has had some time to soak?
(In reply to Vladan Djeric (:vladan) from comment #14)
> Should we uplift this patch to re-enable slowSQL reporting? Maybe after it
> has had some time to soak?

I would rather not. Nightlies will give us enough slowsql data. Would rather not uplift possible string-parsing bugs.
You need to log in before you can comment on or make changes to this bug.