1159385 - pdf.js mochitests are permafail in e10s mode

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect, P2)

Description

[Ryan VanderMeulen [:RyanVM]]

The pdf.js mochitests are currently disabled on e10s with a comment in the manifest pointing to bug 942707. That bug has been long-fixed, but apparently the tests were left disabled. Try runs confirm that they're still permafail.

Filing this bug so we can properly track fixing and re-enabling them.

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

41 INFO TEST-UNEXPECTED-FAIL | browser/extensions/pdfjs/test/browser_pdfjs_navigation.js | A promise chain failed to handle a rejection: - TypeError: Argument 1 of EventTarget.dispatchEvent does not implement interface Event.
76 INFO TEST-UNEXPECTED-FAIL | browser/extensions/pdfjs/test/browser_pdfjs_zoom.js | Content has correct zoom - Got 125%, expected Automatic Zoom
80 INFO TEST-UNEXPECTED-FAIL | browser/extensions/pdfjs/test/browser_pdfjs_zoom.js | A promise chain failed to handle a rejection: - TypeError: Argument 1 of EventTarget.dispatchEvent does not implement interface Event.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make browser_pdfjs_main.js e10s compatible

Hey Mike, do you know if there's a better way to do this? I was hoping to be able to use a single ContentTask, but the way things are set up, that turned out to be a pain (no nice test utils like ok or is). If this looks sane enough to you, the rest of these tests should be easy to convert.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8671112 [details] [diff] [review]
Make browser_pdfjs_main.js e10s compatible

Review of attachment 8671112 [details] [diff] [review]:
-----------------------------------------------------------------

Lovely!

Bonus points if you just dump all of that runTests stuff inside the withNewTab function - I don't really see that leaving it split out adds much.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make browser_pdfjs_main.js e10s compatible.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make browser_pdfjs_navigation.js e10s-compatible.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make browser_pdfjs_views.js e10s-compatible.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make browser_pdfjs_zoom.js e10s-compatible.

Unfortunately, the control flow here was too complicated to keep all of the is/ok tests in the parent. I got around that by pushing the tests into an array and running them in the parent. It isn't ideal, but it works. Also, we don't have EventUtils in the child, so I had to use KeyboardEvent directly.  This meant hardcoding the keyCode, which is ugly, but oh well.

Comment 7

[Yury Delendik (:yury)]

Comment on attachment 8672880 [details] [diff] [review]
Make browser_pdfjs_navigation.js e10s-compatible.

Review of attachment 8672880 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thank you.

Comment 8

[Yury Delendik (:yury)]

Comment on attachment 8672881 [details] [diff] [review]
Make browser_pdfjs_views.js e10s-compatible.

Review of attachment 8672881 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 9

[Yury Delendik (:yury)]

Comment on attachment 8672882 [details] [diff] [review]
Make browser_pdfjs_zoom.js e10s-compatible.

Review of attachment 8672882 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. See if extracting pagerendered wait logic will let you move ok-checks inside of parent.

::: browser/extensions/pdfjs/test/browser_pdfjs_zoom.js
@@ +118,5 @@
> +            if(e.detail.pageNumber !== 1) {
> +              return;
> +            }
> +
> +            document.removeEventListener("pagerendered", onPageRendered, true);

document.removeEventListener/addEventListener("pagerendered"... block appears to be async waiting to specific page to be rendered -- I wonder if extracting that as a function returning promise will make the logic more readable.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Make browser_pdfjs_zoom.js e10s-compatible.

This is so much nicer. I'm going to land this patch-set (with the fix for bug 1165558) now. I'll fix any last comments you have after the fact.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/863379c3fe31
https://hg.mozilla.org/integration/mozilla-inbound/rev/50b20aa3fd46
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dcf1d5a95ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c0a6b934c7a

Comment 12

[Yury Delendik (:yury)]

Comment on attachment 8673415 [details] [diff] [review]
Make browser_pdfjs_zoom.js e10s-compatible.

Review of attachment 8673415 [details] [diff] [review]:
-----------------------------------------------------------------

Really nice. Thank you.

Comment 13

[Phil Ringnalda (:philor)]

Backed out in a6f277178c7b for making the bug 1016737 leak-until-shutdown very very frequent on Windows.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

These patches got bitrotted by the patch that landed in bug 1016737, but the good news is that it hopefully fixed the leaks that got them backed out last time. Up for rebasing, Blake? :)

Comment 15

[Yury Delendik (:yury)]

Bug 1016737 adds waiting for destruction of the PDF viewer: to finish worker and in-flight promises that produce promises. I skipped browser_pdfjs_navigation.js due to the issue fixed in bug 1222198.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

I've been working on other priorities but definitely will be getting back to this soon.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f576ffa8f12f

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rebased patch

This patch updates all of the tests to use the new PDFViewerApplication.close function. It also uses the fact that billm added is() and ok() to ContentTasks.to make control flow a little nicer and to reduce redundancy.

The only test that ended up being problematic was browser_pdfjs_zoom.js.  I slightly changed the ordering of when we got the "zoom=1" container width and when we hit the event loop causing us to do our computations against the wrong value. I fixed this by explicitly setting the initial zoom to 100% so we don't do any sort of resizing.

I don't have an interdiff, unfortunately. Sorry.

Comment 19

[Yury Delendik (:yury)]

Comment on attachment 8686907 [details] [diff] [review]
Rebased patch

Review of attachment 8686907 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I would add "yield viewer.close();" to the browser_pdfjs_navigation.js for same reason why it was added to the all other test (if it works fine during testing).

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25a5db43969ccfcac8d71f965bfc76f53b15fddb
Bug 1159385 - Make PDFJS browser-chrome tests e10s compatible. r=yury

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/25a5db43969c

Comment 22

[Nigel Babu [:nigelb]]

Backed out in

https://hg.mozilla.org/mozilla-central/rev/4294bf91174b

This bug was backed out for not entirely it's own fault, but it's caused weirdness in browser-chrome tests (mostly) on windows PGO. When philor disabled a set of tests in https://hg.mozilla.org/integration/fx-team/rev/41b5c629c4a0, it caused bc4 perma orange and when he disabled another set of tests in https://hg.mozilla.org/integration/fx-team/rev/5fb6f8b316ca, the perma-orange shifted to bc1. I've talked to jmaher and Gijs and we're all stumped how this could happen. Since disables are not at fault and since this test has been causing more than a few oranges, I'm backing this out.

< Gijs> nigelb|sheriffduty: so I think I know what it is, just not entirely sure how to fix it
< Gijs> nigelb|sheriffduty: all these pdfjs tests run a content task waiting for an event that is presumably generated by pdfjs
< Gijs> https://dxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/test/head.js
< Gijs> I bet that event is firing after the content task is attached

< Gijs> nigelb|sheriffduty: yeah, I can't get that test to behave properly in a reasonable timeframe

In light of the situation above and these conversations with Gijs, I've backed this out in agreement with Tomcat.

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix intermittant oranges

This patch applies on top of attachment 8686907 [details] [diff] [review] - it attaches the event listener in content before we load the pdf at all, fixing the race.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cf27eb1dc8a

I don't know if that try run is running the tests with the right settings, but they're not failing, at least.

Comment 24

[Yury Delendik (:yury)]

Comment on attachment 8688580 [details] [diff] [review]
Fix intermittant oranges

Looks good. Let's try this method.

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Rolled-up patch

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccb27d336e71

Comment 27

[Carsten Book [:Tomcat]]

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