1525051 - Fix resource-timing/buffer-full-inspect-buffer-during-callback.html WPT test

Status: RESOLVED FIXED

(Core :: Performance, defect)

Description

[Jan de Mooij [:jandem]]

See bug 1522837 comment 18. This test is hitting overrecursions and timeouts. I'm disabling it because with a very small interpreter refactoring patch it actually starts failing instead of timing out on Linux32 Opt. I don't think it's testing what it should be testing.

Comment 1

[Jan de Mooij [:jandem]]

(Disabling this test in bug 1522837.)

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

The test is for performance API. I'm moving this to Performance module to ensure this is on their radar.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 66 since this is causing a permafail in beta right now.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Vicky, can you help find an owner? It would be helpful for relman, sheriffs, etc if we could resolve these test failures in beta 66.

Comment 5

[Vicky Chin [:vchin]]

Will, can you take a look at the test in question here and assess for correctness?

Comment 6

[Will Hawkins]

This is a bug in the wpt itself. There is an off-by-one error in this particular test. I am going to submit a request upstream so that they can fix the problem.

Comment 7

[Will Hawkins]

(In reply to Will Hawkins from comment #6)

This is a bug in the wpt itself. There is an off-by-one error in this particular test. I am going to submit a request upstream so that they can fix the problem.

Okay, I was too quick to say that this was an off-by-one error in the test. It is actually a problem with our implementation of the Performance API.

We do not implement the "Extensions to the Performance Interface" (https://w3c.github.io/resource-timing/#sec-extensions-performance-interface) which specifies that there is a secondary buffer that queues up new resource additions while the resourcetimingbufferfull event runs.

I am going to take care of implementing this.

Comment 8

[Will Hawkins]

Attachment: WIP: Makes test pass, but includes lots of debugging and verbose comments.

WIP patch that implements the spec, but has lots of debugging printf() statements and verbose commentary. I will clean this up.

Comment 9

[Will Hawkins]

Attachment: WIP: Makes test pass, but includes lots of debugging and verbose comments.

This WIP builds on the previous version to fix an issue where entries are pulled from the secondary queue into the primary queue in reverse order.

Comment 10

[Will Hawkins]

FYI: By fixing this test, we will also "fix" several other WPT that we have previously marked as expected failures. More on this later.

Comment 11

[Will Hawkins]

Attachment: Bug 1525051: Fix resource-timing/buffer-full-inspect-buffer-during-callback.html WPT test.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Did this ever land?

Comment 13

[Will Hawkins]

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #12)

Did this ever land?

:lizzard, I have been OOO at onboarding all week. I got the review from phabricator and I will attempt to get a revision made and submitted by the end of the weekend but COB Monday, at the latest. I hope that's okay!

Will

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

[Tracking Requested - why for this release]:

Looking this over, I think it may be better to let it stay in 67 than to try to rush it to 66 beta. Looks like it will be a big improvement for beta though to have more WPT tests working!

Comment 15

[Andrea Marchesini [:baku]]

Comment on attachment 9046584 [details]
Bug 1525051: Fix resource-timing/buffer-full-inspect-buffer-during-callback.html WPT test.

I didn't see the review request. Thanks for this feedback request. The patch looks good!

Comment 16

[Will Hawkins]

Comment on attachment 9046584 [details]
Bug 1525051: Fix resource-timing/buffer-full-inspect-buffer-during-callback.html WPT test.

Sorry to have to ask for another review, but we found a problem with this patch when we were testing today. In addition, we added to this patch by deleting the .ini files for tests that are now supported thanks to this change.

Again, sorry to have ask again! I appreciate your help!
Will

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:whawkins, could you have a look please?

Comment 18

[Will Hawkins]

We are ready for check in on this.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cd466be3bacb
Fix resource-timing/buffer-full-inspect-buffer-during-callback.html WPT test. r=baku

Comment 20

[Dorel Luca [:dluca]]

Backed out changeset cd466be3bacb (bug 1525051) for mochitest failures in dom/tests/mochitest/general/test_performance_timeline.html. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=237792309&repo=autoland&lineNumber=9991

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=cd466be3bacba2e0cb748b4eb5c0087513403e49

Backout:
https://hg.mozilla.org/integration/autoland/rev/e488d33343f06034fb65c13f682ae42bcc8fd32a

Comment 21

[Will Hawkins]

Fix has been made and is now going through the review process.

Comment 22

[Will Hawkins]

Attachment: Bug 1525051: Fix resource-timing/buffer-full-inspect-buffer-during-callback.html WPT test.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc8e28a50d78
Fix resource-timing/buffer-full-inspect-buffer-during-callback.html WPT test. r=baku

Comment 24

[Noemi Erli[:noemi_erli]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Hi Will, did you want to nominate this for Beta uplift since it's tracking+ for 67?

Comment 26

[Will Hawkins]

This does not seem like a bad idea. I would prefer to wait until we land a separate, but complementary patch, and then nominate them both. How does that sound? The one that we are waiting on is 1539006. Thanks for asking about this!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

That looks like possibly more than we'd want to take when we're already near the midpoint of the Beta cycle. Redirecting to Pascal since he's the 67 release owner.

Comment 28

[Pascal Chevrel:pascalc]

I am not sure I would take bug 1539006 indeed

Comment 29

[Will Hawkins]

Because this is a resolved issue, are you adding this as a blocker for the meta bug just for documentation purposes? Just wanted to make sure I am not missing a "signal" that there is a regression or other problem.

Thanks!

Comment 30

[David Burns :automatedtester]

Sorry, I cloned a bug that had these as dependencies and I wasnt careful. Sorry again!

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Doesn't sound like we're going to take this in 67 given where we are in the cycle and the dependency on bug 1539006. If you feel strongly otherwise, please nominate for approval ASAP.