PerformanceObserver should not throw on empty/unknown list of entryTypes, but silently ignore the error (with optional console warning).
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
People
(Reporter: wisniewskit, Assigned: wisniewskit)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
The WG just resolved that creating a PerformanceObserver with no known entryTypes should not throw an exception, but should simply create a useless observer. (The UA may log an optional console warning, however): - https://github.com/w3c/performance-timeline/issues/87#issuecomment-332050411 - https://github.com/w3c/performance-timeline/pull/88
Comment 1•7 years ago
|
||
This seems at Hiroyuki-san's wheelhouse. Looping him in. :)
Assignee | ||
Comment 2•7 years ago
|
||
I actually have a patch almost ready, I'm just waiting for the related web platform test changes to be merged in https://github.com/w3c/web-platform-tests/pull/7474, and then I'll submit it.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years ago
|
||
The test-changes were just merged, so I've gone ahead and posted my patch for review. Note that my patch incorporates that updated test-file from the WPT master branch, and also updates the metafiles for the various tests which must now TIMEOUT instead of FAILing (because they don't throw on unknown entryTypes anymore, but rather silently fail). The patch seems to pass try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c121fec099f7dcb37ab4a904b3ddb306ab6ab3ca
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
I've just updated the patch to also log a web console warning (I don't see any existing tests covering such warnings, so I didn't add a test for it).
![]() |
||
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8914579 [details] Bug 1403027 - Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); https://reviewboard.mozilla.org/r/185920/#review191496 r=me ::: dom/performance/PerformanceObserver.cpp:176 (Diff revision 2) > + addComma = true; > + invalidTypesJoined.Append(type); > + } > + } > + > + if (invalidTypesJoined.Length()) { !invalidTypesJoined.IsEmpty()
Assignee | ||
Comment 8•7 years ago
|
||
Actually pardon me, bz, but it seems I missed the worker case for the console logging code. I'm running a new try-run with an updated patch, and will request a new review as soon as that seems clear. (Glad I caught that before submitting it for check-in)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
The try-run looks fine, so I've posted an updated patch to ReviewBoard: https://treeherder.mozilla.org/#/jobs?repo=try&revision=79f1fd3d6162d68c46ac68bdc7d2a6f58735a0fb bz, would you mind looking over the patch interdiff just in case? I addressed your review comment, a silly bug I somehow missed where I should have used AppendLiteral, and the rest is basically about supporting logging properly in the worker case.
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8914579 [details] Bug 1403027 - Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); https://reviewboard.mozilla.org/r/185920/#review192022 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/performance/PerformanceObserver.cpp:166 (Diff revision 3) > } > } > > + nsAutoString invalidTypesJoined; > + bool addComma = false; > + for (auto type : aOptions.mEntryTypes) { Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy] for (auto type : aOptions.mEntryTypes) { ^ const &
![]() |
||
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8914579 [details] Bug 1403027 - Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); https://reviewboard.mozilla.org/r/185920/#review192024 ::: dom/workers/WorkerPrivate.cpp:979 (Diff revision 3) > } > > // Now fire a runnable to do the same on the parent's thread if we can. > if (aWorkerPrivate) { > RefPtr<ReportErrorToConsoleRunnable> runnable = > - new ReportErrorToConsoleRunnable(aWorkerPrivate, aMessage); > + new ReportErrorToConsoleRunnable(aWorkerPrivate, aMessage, Is this a sync runnable? As in, do we not return until after it has run? Because if not, nothing is keeping aParams alive until the runnable actually runs... and I don't think this is a sync runnable. aMessage has the same problem in theory, but in practice it gets away with it because all callers pass literal strings.
![]() |
||
Comment 13•7 years ago
|
||
And presumably there are no tests exercising this codepath in workers with unknown types...
Assignee | ||
Comment 14•7 years ago
|
||
>nothing is keeping aParams alive until the runnable actually runs Ah, you're right. I'll go ahead and change the parameters to pass in nsTArray<nsString>s (as other runnables seem to be doing in WorkerPrivate.cpp) and make sure it holds its own copy of mParams. Should I also change the existing aMessage parameter to pass in an nsString and store its own reference, while I'm at it? >presumably there are no tests exercising this codepath in workers with unknown types... I'd like to add tests for that, but I don't actually see any existing uses of nsContentUtils::ReportToConsole or WorkerPrivate::ReportErrorToConsole being tested in an obvious place, so I'm not sure how I would go about testing this error-logging codepath. Thoughts?
![]() |
||
Comment 15•7 years ago
|
||
SimpleTest.expectConsoleMessages is probably the simplest approach to watching the console in a test. Can't be done in a web platform test, obviously... So I would say we should ideally have: 1) A web platform test that tests that in a worker context the API behaves correctly (e.g. doesn't throw when the input is all-unsupported). I didn't look carefully to see whether the existing test can just become a .any.js test; if it can that would be the simplest way to achieve this. 2) A mochitest using SimpleTest.expectConsoleMessages, or SimpleTest.monitorConsole/endMonitorConsole to watch for the right messages coming through. If you're not all up for doing all that, then at least some test that at least exercises the code, even if it doesn't check how it ends up behaving, would be good.
Assignee | ||
Comment 16•7 years ago
|
||
Ah, I see what you mean now. I have no problem making sure the tests are as solid as possible; I'll try to get them done ASAP.
Assignee | ||
Comment 17•7 years ago
|
||
By the by, the WPT for this does have ".any" in its filename, and does test both worker and non-worker cases (aside from the console logging), so I'll just be adding the mochitests for console logging.
![]() |
||
Comment 18•7 years ago
|
||
Hmm. I guess we got (un?)lucky with that test not crashing in the worker case, then...
Assignee | ||
Comment 19•7 years ago
|
||
It's far more likely that it's just because I didn't test it properly after making my console-logging changes. Good thing I'm taking some PTO next week, I guess :) I'm doing one more try-run of the corrected version here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc24884136cd52130f8fbd621108e60f607cc698 As I mentioned in comment 14, this version uses nsTArray<nsString> to keep the data alive until the worker runs, and adds two mochitests to confirm that logging takes place (one for the worker case, and one for the non-worker case). Assuming it's fine, I'll submit it for review when I get to work tomorrow. Thanks again, bz!
![]() |
||
Comment 20•7 years ago
|
||
Oh, I never responded to the question about what types to use. nsTArray<nsString> for the params makes sense. nsCString might make sense for the message...
Assignee | ||
Comment 21•7 years ago
|
||
Ah, ok, then I will make one more adjustment to change the message to an nsCString as well (sorry, I presumed you didn't think it was worth changing that in this patch; I'll make sure to ask in the future).
![]() |
||
Comment 22•7 years ago
|
||
Oh, you can leave message as const char* too, for now. Maybe file a followup on considering changing it. For now, as long as we use static strings, it's safe.
Assignee | ||
Comment 23•7 years ago
|
||
Alright then, since the try-run passes as-is I'll request check-in, and file a follow-up bug for the message string as you suggest.
Updated•7 years ago
|
Comment 24•7 years ago
|
||
Autoland can't push this because in MozReview it shows as r- with 3 unresolved issues. Please clean that up prior to requesting checkin.
Updated•7 years ago
|
Assignee | ||
Comment 25•7 years ago
|
||
Hmm, I marked the fixed issues, but it looks like it's still r-. bz, was there anything else to do, or is this just something you'll need to rubberstamp?
![]() |
||
Comment 26•7 years ago
|
||
Well, I need to actually review the changes to use the array... ;)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 29•7 years ago
|
||
Oh wow, what happened there... I thought I had pushed the updates for review hours ago! I've done so now. Sorry for the mix-up.
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8914579 [details] Bug 1403027 - Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); https://reviewboard.mozilla.org/r/185920/#review192506 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/performance/PerformanceObserver.cpp:166 (Diff revision 4) > } > } > > + nsAutoString invalidTypesJoined; > + bool addComma = false; > + for (const auto type : aOptions.mEntryTypes) { Warning: The loop variable's type is not a reference type; this creates a copy in each iteration; consider making this a reference [clang-tidy: performance-for-range-copy] for (const auto type : aOptions.mEntryTypes) { ^ &
![]() |
||
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8914579 [details] Bug 1403027 - Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); https://reviewboard.mozilla.org/r/185920/#review192508 ::: dom/performance/PerformanceObserver.cpp:166 (Diff revisions 3 - 4) > } > } > > nsAutoString invalidTypesJoined; > bool addComma = false; > - for (auto type : aOptions.mEntryTypes) { > + for (const auto type : aOptions.mEntryTypes) { Like the static analysis says, "auto&" ::: dom/performance/tests/test_performance_observer.html:24 (Diff revision 4) > var xmlhttp = new XMLHttpRequest(); > xmlhttp.open("get", aUrl, true); > xmlhttp.send(); > } > > +SimpleTest.waitForExplicitFinish(); Hmm.. I don't know how well SimpleTest.waitForExplicitFinish() mixes with testharness tests. Might want to make this a plain mochitest, not a testharness mochitest...
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•7 years ago
|
||
bz, is my revised testcase (to make it a plain mochitest) acceptable? Note that I had to request flaky timeouts, as the harness version used setTimeout. (Also note the non-test changes in there are changes from the patch being rebased, aside from one tweak to the call to ReportToConsole in WorkerPrivate.cpp to not pass in an empty array but rather a nullptr, to keep an assertion from being triggered in the empty-array case).
Assignee | ||
Comment 34•7 years ago
|
||
A try-run seems clean: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c22178d2f4b648e632cb2df57bd5a1ee7f63ff9e
![]() |
||
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8914579 [details] Bug 1403027 - Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); https://reviewboard.mozilla.org/r/185920/#review196886 Looks good in general. ::: dom/performance/tests/test_performance_observer.html:26 (Diff revisions 4 - 5) > +let test = promise_test = fn => { > + let cleanup = () => {}; > + _tests.push(async () => { > + try { > + await fn({ > + add_cleanup: f => { cleanup = f; }, That's OK as long as people don't add_cleanup twice... Could have an array of cleanups. ::: dom/performance/tests/test_performance_observer.html:56 (Diff revisions 4 - 5) > + } > + _tests.shift()(); > +} > + > +function same_value(x, y) { > + if (y !== y) { Is this reinventing Object.is?
![]() |
||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 37•7 years ago
|
||
Alright, thanks bz! I've addressed your comments. Requesting check-in.
Comment 38•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c5f2f0e827d0 Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); r=bz
![]() |
||
Comment 39•7 years ago
|
||
Backed out for build bustage at dom/workers/WorkerPrivate.cpp(984): expression did not evaluate to a constant, at least on Windows: https://hg.mozilla.org/integration/autoland/rev/7e4c85c43cf55d2cd8d299f043fc2eb05e6b2246 Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=c5f2f0e827d03ed659348b586b16493e9588fde2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=138595655&repo=autoland 22:20:00 INFO - Unified_cpp_dom_workers2.cpp 22:20:00 INFO - z:/build/build/src/dom/workers/WorkerPrivate.cpp(984): error C2131: expression did not evaluate to a constant 22:20:00 INFO - z:/build/build/src/dom/workers/WorkerPrivate.cpp(984): note: failure was caused by non-constant arguments or reference to a non-constant symbol 22:20:00 INFO - z:/build/build/src/dom/workers/WorkerPrivate.cpp(984): note: see usage of 'paramCount' 22:20:00 INFO - z:/build/build/src/dom/workers/WorkerPrivate.cpp(986): error C3863: array type 'const char16_t *[paramCount]' is not assignable 22:20:00 INFO - z:/build/build/src/config/rules.mk:1072: recipe for target 'Unified_cpp_dom_workers2.obj' failed 22:20:00 INFO - mozmake.EXE[5]: *** [Unified_cpp_dom_workers2.obj] Error 2
Comment hidden (mozreview-request) |
Assignee | ||
Comment 41•7 years ago
|
||
I've corrected this in the latest patch, which now builds on Windows and still seems to pass try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=198a47f451c38b4acf80cb649d3540273952ddf1 Re-requesting checkin.
Comment 42•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c2c746884242 Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); r=bz
![]() |
||
Comment 43•7 years ago
|
||
Backed out for failing M-e10s(2,h2) without Treeherder failure summary on Windows: https://hg.mozilla.org/integration/autoland/rev/909f3774aa0ca21a64b19f4eac4232b612b938a0 Push with failures (Windows 10 h2): https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b73eab93e81995fb0643b8ab8d4a004bd795ef58&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Push which ran more failing tests: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=89dfd44fcfd43fd7b3131f6d79701595744b95ba&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable If you find the issue in the log, please post it in this bug. Thank you.
![]() |
||
Comment 44•7 years ago
|
||
This also hits Android: https://treeherder.mozilla.org/logviewer.html#?job_id=139636640&repo=autoland
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Alright, I've rebased the patches and fixed the errors, as a fresh try-run confirms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=18ee2eb21f55318e087ae5e8c80cc7f5b75e9f7d It turns out that the major problem was that I was using SimpleTest and testharness.js both at the same time in a worker-related test, which caused it to fail in odd ways. I was doing that in order to read the console log, but this new version just as easily does so without SimpleTest. bz, given the time that has passed, do you feel this could use a fresh review? (pardon the rebase-including interdiff).
Updated•6 years ago
|
![]() |
||
Comment 48•6 years ago
|
||
mozreview-review |
Comment on attachment 8914579 [details] Bug 1403027 - Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); https://reviewboard.mozilla.org/r/185920/#review261440 ::: dom/locales/en-US/chrome/dom/dom.properties:361 (Diff revision 8) > OrientationEventWarning=Use of the orientation sensor is deprecated. > ProximityEventWarning=Use of the proximity sensor is deprecated. > AmbientLightEventWarning=Use of the ambient light sensor is deprecated. > # LOCALIZATION NOTE: Do not translate "storage", "indexedDB.open" and "navigator.storage.persist()". > IDBOpenDBOptions_StorageTypeWarning=The ‘storage’ attribute in options passed to indexedDB.open is deprecated and will soon be removed. To get persistent storage, please use navigator.storage.persist() instead. > -DOMQuadBoundsAttrWarning=DOMQuad.bounds is deprecated in favor of DOMQuad.getBounds() > +UnsupportedEntryTypesIgnored=Ignoring unsupported entryTypes: %S. Why is this removing DOMQuadBoundsAttrWarning? It should stay... r=me with that problem fixed.
![]() |
||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Comment 51•6 years ago
|
||
Pushed by ccoroiu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f839685ceb8e Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead); r=bz
Comment 52•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f839685ceb8e
Updated•6 years ago
|
Comment 54•6 years ago
|
||
Corrected the explanation of the functionality of observe(): https://developer.mozilla.org/en-US/docs/Web/API/PerformanceObserver/observe Listed on Firefox 63 for developers.
Comment 55•6 years ago
|
||
Thanks to the team for your work.
I have FireFox 66 and get red 'TypeError: The expression cannot be converted to return the specified type' messages that are not bound to any JS code line.
Since this error was removed for PerformanceObserver (and I am not running "about:performance", though this may be irrelevant), this probably means that regular MutationObserver causes the error or something else entirely?
Or if the JS line number and code link/referrer are not shown along with the error, does it mean that this is FF's own code's error?
![]() |
||
Comment 56•6 years ago
|
||
That's the generic message for TypeError, and TypeError is the most common sort of error various specifications throw.
If you have steps to reproduce (or at least a site URL?) please file a new bug about it and cc me, and I'll take a look.
Comment 57•6 years ago
|
||
Thanks, Boris I will try to catch it again.
In the meantime, if I may ask a question, do you know why it is supposed to be a legitimate behaviour for errors to have "this expression" right in their textual representation and yet fail to point to any expression in any way?
Should not it be in a violation of arguments/properties "contract" in regards to this error? It must have either a pointer to where the expression is placed in code or an expression literal at least.
And if this behaviour is normal, should not an "issue" be filed to change the phrasing of the error?
![]() |
||
Comment 58•6 years ago
|
||
That particular string is attached to NS_ERROR_DOM_TYPE_ERR, which shouldn't even exist. See 927610 comment 2. Any time you see it, someone messed up and is throwing a DOMException with name set to "TypeError" instead of a real TypeError. We need to fix that, but I haven't had time to fight with the broken test harness so far to make it happen.
That said, that wouldn't affect whether location information is attached to it or not.
Updated•6 years ago
|
Description
•