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

Status: RESOLVED FIXED

(Testing :: Mochitest, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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 -

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Brendan Dahl [: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.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Chris Peterson [:cpeterson]]

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.

Comment 5

[Andrew Halberstadt [:ahal]]

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.

Comment 6

[Andrew McCreight [:mccr8]]

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

Comment 7

[Chris Peterson [:cpeterson]]

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

Comment 8

[Chris Peterson [:cpeterson]]

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

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Andrew McCreight [:mccr8]]

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

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Bug 1661173 - Use browserId instead of the BC for BrowserTestUtils.addContentEventListener.

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/993c84c21b4e
Use browserId instead of the BC for BrowserTestUtils.addContentEventListener. r=kmag

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/993c84c21b4e

Comment 14

[Andrew McCreight [:mccr8]]

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

Comment 15

[Andrew McCreight [:mccr8]]

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 16

[Ryan VanderMeulen [:RyanVM]]

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

Approved for 81.0b7.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cf9c6bd872aa

Comment 18

[Intermittent Failures Robot]

2 failures in 2984 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• try: 2

Platform breakdown:

• windows7-32-shippable: 1
• windows10-64-shippable: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1661173&startday=2020-08-31&endday=2020-09-06&tree=all