Closed Bug 874743 Opened 11 years ago Closed 11 years ago

Intermittent test_parser_diagnostics_unprintables.html | monitorConsole | extra message | {"message":"[JavaScript Error: \"Not a number\" {file: \"resource://gre/modules/PlacesDBUtils.jsm\" line: 975}]","errorMessage":"Not a number","sourceName":"resource

Categories

(Toolkit :: Places, defect, P5)

Product:
Toolkit
Component:
Places
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28
Tracking Flags:
Tracking Status
firefox26 --- fixed
firefox27 --- fixed
firefox28 --- fixed
firefox-esr24 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: philor, Assigned: adw)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

IFNULLs, more logging
3.19 KB, patch
mak
: review+
Details | Diff | Splinter Review 
So, you thought you completely owned the console while your test was running, did you?

https://tbpl.mozilla.org/php/getParsedLog.php?id=23227437&tree=Mozilla-Central
Ubuntu VM 12.04 mozilla-central pgo test mochitest-5 on 2013-05-21 15:34:05 PDT for push c96b50b62e75
slave: tst-linux32-ec2-090

15:38:48     INFO -  34357 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_parser_diagnostics_unprintables.html | monitorConsole | extra message | {"message":"[JavaScript Error: \"Not a number\" {file: \"resource://gre/modules/PlacesDBUtils.jsm\" line: 975}]","errorMessage":"Not a number","sourceName":"resource://gre/modules/PlacesDBUtils.jsm","sourceLine":"","lineNumber":975,"columnNumber":0,"category":"XPConnect JavaScript","windowID":0,"isScriptError":true,"isWarning":false,"isException":false,"isStrict":false}
Chrome shouldn't just spit out random JS errors, either.  I'm inclined to say this is the fault of that exception rather than the fault of the test.
Component: CSS Parsing and Computation → Places
Product: Core → Toolkit
Component: Places → CSS Parsing and Computation
Product: Toolkit → Core
I think it's infeasible to avoid the "spamming to the console randomly" cases entirely, so I think the test should handle that possibility (like many other tests have).
Many others have not, and I've taken to just throwing things into the Core::General garbage can if they involve this astonishingly tiresome situation.
(In reply to Phil Ringnalda (:philor) from comment #137)
> Many others have not, and I've taken to just throwing things into the
> Core::General garbage can if they involve this astonishingly tiresome
> situation.

Send me a list?
Priority: -- → P5
Assignee: nobody → adw
Status: NEW → ASSIGNED
No longer blocks: 229827
Depends on: 900953
This is the same failure as bug 900953 and so should be fixed by its landing.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
status-firefox26: --- → fixed
status-firefox27: --- → fixed
status-firefox28: --- → fixed
status-firefox-esr24: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(In reply to Drew Willcoxon :adw from comment #140)
> This is the same failure as bug 900953 and so should be fixed by its landing.

About that... :(
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch typeof == "number", not isFinite (obsolete) — — Splinter Review 
Fixing bug 900953 using isFinite() was pretty dumb.  getHistogramById().add() works fine with both NaN and Infinity.  typeof == "number" is what I should have used.

The error is thrown here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#615

I can't really tell how or whether JS::Value::isNumber is reflected to JS, but typeof == "number" seems like it might imply isNumber() is true.
Attachment #8335797 - Flags: review?(mak77)
Comment on attachment 8335797 [details] [diff] [review]
typeof == "number", not isFinite

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

I see, the issue seems to be that division by zero in a sqlite query returns NULL, and isFinite(null) is true, but it's definitely not a number.
I think this additional check is ok, but in the else brace we should ReportError which probe caused the error, and in this specific case we should fix the PLACES_SORTED_BOOKMARKS_PERC and PLACES_TAGGED_BOOKMARKS_PERC queries (that are dividing by zero) adding an IFNULL(..., 0)
Attachment #8335797 - Flags: review?(mak77) → feedback+
Good idea.  This is slightly different from what you suggested.  It lets any errors get caught by the existing try-catch.  No typeof == "number" check at all, since hopefully the IFNULLs prevent any more errors, but if not, the additional logging should help us figure them out.
Attachment #8335797 - Attachment is obsolete: true
Attachment #8337177 - Flags: review?(mak77)
Attachment #8337177 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/5e2062d62bf6
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Depends on: 948134
Given the patch, I'm reclassifying this as a Toolkit|Places bug.
Component: CSS Parsing and Computation → Places
Product: Core → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: