Thomas Wisniewski Assignee
	Description • 7 years ago

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

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 1 • 7 years ago

This seems at Hiroyuki-san's wheelhouse. Looping him in. :)

	Thomas Wisniewski 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.

	Thomas Wisniewski 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

	Thomas Wisniewski 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).

	Boris Zbarsky [:bzbarsky]
	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()

	Thomas Wisniewski 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)

	Thomas Wisniewski 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.

	Code Review Bot [:reviewbot]
	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  &

	Boris Zbarsky [:bzbarsky]
	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.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 7 years ago

And presumably there are no tests exercising this codepath in workers with unknown types...

	Thomas Wisniewski 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?

	Boris Zbarsky [:bzbarsky]
	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.

	Thomas Wisniewski 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.

	Thomas Wisniewski 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.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 7 years ago

Hmm.  I guess we got (un?)lucky with that test not crashing in the worker case, then...

	Thomas Wisniewski 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!

	Boris Zbarsky [:bzbarsky]
	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...

	Thomas Wisniewski 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).

	Boris Zbarsky [:bzbarsky]
	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.

	Thomas Wisniewski 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.

	Ryan VanderMeulen [:RyanVM]
	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.

	Thomas Wisniewski 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?

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 7 years ago

Well, I need to actually review the changes to use the array... ;)

	Boris Zbarsky [:bzbarsky]
	Comment 27 • 7 years ago

In particular, posting the updated patch would be good.

	Thomas Wisniewski 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.

	Code Review Bot [:reviewbot]
	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) {
                  ^
                 &

	Boris Zbarsky [:bzbarsky]
	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...

	Thomas Wisniewski 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).

	Thomas Wisniewski Assignee
	Comment 34 • 7 years ago

A try-run seems clean: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c22178d2f4b648e632cb2df57bd5a1ee7f63ff9e

	Boris Zbarsky [:bzbarsky]
	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?

	Thomas Wisniewski Assignee
	Comment 37 • 7 years ago

Alright, thanks bz! I've addressed your comments.

Requesting check-in.

	Pulsebot
	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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	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

	Thomas Wisniewski 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.

	Pulsebot
	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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	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.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 44 • 7 years ago

This also hits Android: https://treeherder.mozilla.org/logviewer.html#?job_id=139636640&repo=autoland

	Thomas Wisniewski [:twisniewski]
	Comment 47 • 6 years ago

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).

	Boris Zbarsky [:bzbarsky]
	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.

	Thomas Wisniewski [:twisniewski]
	Comment 50 • 6 years ago

Alright, review point addressed. Thanks bz!

Requesting check-in.

	Pulsebot
	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

	Dorel Luca [:dluca]
	Comment 52 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f839685ceb8e

	Eric Shepherd [:sheppy]
	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.