Closed Bug 1661173 Opened 1 month ago Closed 26 days ago

Perma toolkit/components/pdfjs/test/browser_pdfjs_saveas.js | Test timed out - when Gecko 81 switches to Late Beta on 2020-09-04

Categories

(Testing :: Mochitest, defect, P1)

defect

Tracking

(Fission Milestone:M6b, firefox-esr68 unaffected, firefox-esr78 unaffected, firefox79 unaffected, firefox80 unaffected, firefox81+ fixed, firefox82+ fixed)

RESOLVED FIXED
82 Branch
Fission Milestone M6b
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- unaffected
firefox79 --- unaffected
firefox80 --- unaffected
firefox81 + fixed
firefox82 + fixed

People

(Reporter: aryx, Assigned: mccr8)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

central-as-late-beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=f8c6ba2502241c301f4c44f979ee38aeff304d10&selectedTaskRun=WWAAJvmYQquCaYrtTVp_vw.0

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=313970738&repo=try

[task 2020-08-25T17:19:59.609Z] 17:19:59 INFO - TEST-PASS | toolkit/components/pdfjs/test/browser_pdfjs_saveas.js | File should have been downloaded successfully -
[task 2020-08-25T17:19:59.609Z] 17:19:59 INFO - Buffered messages finished
[task 2020-08-25T17:19:59.610Z] 17:19:59 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/pdfjs/test/browser_pdfjs_saveas.js | Test timed out -

Flags: needinfo?(bdahl)
Assignee: nobody → bdahl
Flags: needinfo?(bdahl)
Regressed by: 1659630

@ Paul, do you know why this pdf.js test would start timing out with fission.preserve_browsing_contexts resets from true to false in (simulated) "late beta" Fx81?

@ Brendan, does this test also time out in Fx82 Nightly if fission.preserve_browsing_contexts is set to false? I'm wondering whether this is a bug only in Fx81 and/or Beta.

Tentatively tracking for Fission milestone M6c because this bug is fallout from a Fission change.

Fission Milestone: --- → M6c
Flags: needinfo?(pbone)
Flags: needinfo?(bdahl)

(In reply to Chris Peterson [:cpeterson] from comment #1)

@ Brendan, does this test also time out in Fx82 Nightly if fission.preserve_browsing_contexts is set to false? I'm wondering whether this is a bug only in Fx81 and/or Beta.

The test hangs in mozcentral (61ed3192760a3aaef282c089c064fc8dd3890125) with fission.preserve_browsing_contexts=false for me too.

Flags: needinfo?(bdahl)

(In reply to Chris Peterson [:cpeterson] from comment #1)

@ Paul, do you know why this pdf.js test would start timing out with fission.preserve_browsing_contexts resets from true to false in (simulated) "late beta" Fx81?

Brendan said it fails on this line, and that the test involves navigating to or from a file URL: https://searchfox.org/mozilla-central/rev/27932d4e6ebd2f4b8519865dad864c72176e4e3b/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#1375

Just looking at this line, it makes sense that it would break with BC preservation disabled, because without it we change BC on navigation so this will return early and not report the event. This is effectively a defect in BrowserTestUtils.waitForContentEvent() with browsing context disabled, which nobody noticed before because nobody was hitting this particular configuration before.

In the Fission channel on Element, kmag said: "It could be changed to check the embedder element instead."

Flags: needinfo?(pbone)

This is effectively a defect in BrowserTestUtils.waitForContentEvent() with browsing context disabled, which nobody noticed before because nobody was hitting this particular configuration before.

In the Fission channel on Element, kmag said: "It could be changed to check the embedder element instead."

In that case, I'll move this bug to the Testing::Mochitest component.

@ ahal, this is a BrowserTestUtils bug that will cause a pdf.js test failure once Fx81 Beta enters the "late beta" period on September 4.

Severity: -- → N/A
Fission Milestone: M6c → M6b
Component: PDF Viewer → Mochitest
Flags: needinfo?(ahal)
OS: Unspecified → All
Priority: -- → P1
Product: Firefox → Testing
Hardware: Unspecified → All
See Also: → 1586393

Thanks for the heads up! I'll be honest, Andrew's comment 3 sounds Greek to me (I'd love if we could coerce someone with knowledge about the JS half of Mochitest to become a peer). Do you know if this is the only test affected?

Fyi, our team was severely affected by the layoff, so our ability to dig into stuff like this (especially with limited JS / frontend expertise) is very low at the moment. I could probably only commit to disabling the test by the 4th.

I'm willing to attempt a patch with hand holding, but at that point it's likely faster if whoever was holding my hand did it themselves. I'm skeptical I'd be done by the 4th at any rate.

Flags: needinfo?(ahal)

The easiest workaround would be to enable browsing context preservation for this test.

I do that in one other place: https://searchfox.org/mozilla-central/rev/ce21a13035623c1d349980057d09000e70669802/dom/tests/browser/browser_windowProxy_transplant.js#17-19

(In reply to Andrew McCreight [:mccr8] from comment #6)

The easiest workaround would be to enable browsing context preservation for this test.

Good point! fission.preserve_browsing_contexts is enabled by default in Nightly and, if all goes well, will ride the trains in Fx82 or Fx83. So we probably shouldn't spend a lot of time fixing BrowserTestUtils to work when fission.preserve_browsing_contexts is disabled in Beta and Release for just one or two release cycles.

Moving this bug back to the PDF Viewer component because the recommended fix is to patch the pdf.js test (to temporarily set the fission.preserve_browsing_contexts pref = true in the test) itself instead of fixing mochitest's BrowserTestUtils.waitForContentEvent function.

@ ahal, thus you and your team aren't responsible for fixing this bug. :)

@ Paul, since you landed the Browsing Context Preservation changes, can you please patch the pdf.js test like mccr8 did for the browser_windowProxy_transplant.js test?

Unless bdahl, as the PDF Viewer component owner, prefers to fix the test himself?

For a few release cycles now, Browsing Context Preservation has been enabled in Nightly and not in Beta. Why did this test pass in Fx80 Beta without Browsing Context Preservation but will fail in Fx81 Beta when Browsing Context Preservation is turned off in late beta? Did this test change during the Fx81 Nightly cycle?

(In reply to Andrew McCreight [:mccr8] from comment #6)

The easiest workaround would be to enable browsing context preservation for this test.

I do that in one other place: https://searchfox.org/mozilla-central/rev/ce21a13035623c1d349980057d09000e70669802/dom/tests/browser/browser_windowProxy_transplant.js#17-19

Component: Mochitest → PDF Viewer
Flags: needinfo?(pbone)
Flags: needinfo?(bdahl)
Product: Testing → Firefox
Assignee: bdahl → continuation
Status: NEW → ASSIGNED
Flags: needinfo?(pbone)
Flags: needinfo?(bdahl)

We talked about this in the Fission meeting. Nika and kmag said it would be easy to fix the testing function, so I'll do that. The fix that Nika suggested was to check the BC id instead. kmag said that because the argument to this method is a browser, it will be a top level BC, so that should be sufficient.

Component: PDF Viewer → Mochitest
Product: Firefox → Testing

kmag followed up and told me that it is actually the "browser ID", which is stable, and not the "browsing context ID".

The browserId is stable across navigations, but the browsing context
itself might not be if we are not preserving the BC across navigations.

This change means we would start getting notified about events for
subframes, which share the browserID but not the BC of the top level
frame, so I added code to ignore those.

Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/993c84c21b4e
Use browserId instead of the BC for BrowserTestUtils.addContentEventListener. r=kmag
Status: ASSIGNED → RESOLVED
Closed: 26 days ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch

Comment on attachment 9173706 [details]
Bug 1661173 - Use browserId instead of the BC for BrowserTestUtils.addContentEventListener.

Beta/Release Uplift Approval Request

  • User impact if declined: None. This is a test-only fix. (I'm not sure how to flag this for checkin on beta without asking for approval.)
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Test only change
  • String changes made/needed: none
Attachment #9173706 - Flags: approval-mozilla-beta?

I think BC preservation might be riding the trains, so this won't really be needed, but we might as well backport the fix so that this test passes either way in case that ends up falling through.

Comment on attachment 9173706 [details]
Bug 1661173 - Use browserId instead of the BC for BrowserTestUtils.addContentEventListener.

Approved for 81.0b7.

Attachment #9173706 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.