Closed Bug 1403027 Opened 8 years ago Closed 7 years ago

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)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox63 --- fixed

People

(Reporter: wisniewskit, Assigned: wisniewskit)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

Bug 1403027 - Do not throw from PerformanceObserver.observe when none of the entryTypes are known (log a JS console warning instead);
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
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
This seems at Hiroyuki-san's wheelhouse. Looping him in. :)
Priority: -- → P3
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.
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
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 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()
Attachment #8914579 - Flags: review?(bzbarsky) → review+
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)
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.
Flags: needinfo?(bzbarsky)
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 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.
Attachment #8914579 - Flags: review+ → review-
And presumably there are no tests exercising this codepath in workers with unknown types...
Flags: needinfo?(bzbarsky)
>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?
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
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.
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.
Hmm. I guess we got (un?)lucky with that test not crashing in the worker case, then...
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!
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...
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).
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.
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.
Keywords: checkin-needed
Assignee: nobody → wisniewskit
Autoland can't push this because in MozReview it shows as r- with 3 unresolved issues. Please clean that up prior to requesting checkin.
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?
Flags: needinfo?(bzbarsky)
Well, I need to actually review the changes to use the array... ;)
Flags: needinfo?(bzbarsky)
In particular, posting the updated patch would be good.
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 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 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...
Attachment #8914579 - Flags: review?(bzbarsky) → review+
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).
Flags: needinfo?(bzbarsky)
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?
Flags: needinfo?(bzbarsky)
Alright, thanks bz! I've addressed your comments. Requesting check-in.
Keywords: checkin-needed
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
Keywords: checkin-needed
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
Flags: needinfo?(wisniewskit)
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.
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
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
Keywords: checkin-needed
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).
Flags: needinfo?(wisniewskit)
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
Alright, review point addressed. Thanks bz! Requesting check-in.
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f839685ceb8e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
Keywords: dev-doc-needed → dev-doc-complete

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?

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.

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?

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.

Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: