Closed Bug 1525051 Opened 4 years ago Closed 3 years ago

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

Categories

(Core :: Performance, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox66 + wontfix
firefox67 + wontfix
firefox68 --- fixed

People

(Reporter: jandem, Assigned: whawkins)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.

(Disabling this test in bug 1522837.)

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

Component: DOM → Performance

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

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.

Flags: needinfo?(vchin)

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

Assignee: nobody → whawkins
Flags: needinfo?(vchin)

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.

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

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

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.

Attachment #9046094 - Attachment is obsolete: true

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

Did this ever land?

Flags: needinfo?(whawkins)

(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

Flags: needinfo?(whawkins)

[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!

Attachment #9046584 - Flags: feedback?(amarchesini)

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!

Attachment #9046584 - Flags: feedback?(amarchesini)

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

Attachment #9046584 - Flags: feedback?(amarchesini)
Blocks: 1522621, 1522537
Attachment #9046584 - Flags: feedback?(amarchesini) → feedback+

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?

Flags: needinfo?(whawkins)

We are ready for check in on this.

Flags: needinfo?(whawkins)
Keywords: checkin-needed
Status: NEW → ASSIGNED

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

Keywords: checkin-needed

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

Flags: needinfo?(whawkins)
Depends on: 1542342
Attachment #9046584 - Flags: feedback+
Attachment #9056293 - Flags: feedback?(mstange)
Attachment #9056293 - Flags: feedback?(amarchesini)
Attachment #9056293 - Flags: feedback?(mstange)
Attachment #9056293 - Flags: feedback?(amarchesini)
Keywords: checkin-needed

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

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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

Flags: needinfo?(whawkins)
Flags: in-testsuite+

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!

Flags: needinfo?(whawkins) → needinfo?(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.

Flags: needinfo?(ryanvm) → needinfo?(pascalc)

I am not sure I would take bug 1539006 indeed

Flags: needinfo?(pascalc)
Blocks: 1528805
Blocks: 1528798
Blocks: 1525025
Blocks: 1513292

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!

Flags: needinfo?(dburns)
No longer blocks: 1545049

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

Flags: needinfo?(dburns)

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.

You need to log in before you can comment on or make changes to this bug.