Closed Bug 1159385 Opened 6 years ago Closed 6 years ago

pdf.js mochitests are permafail in e10s mode

Categories

(Firefox :: PDF Viewer, defect, P2)

38 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
e10s + ---
firefox45 --- fixed

People

(Reporter: RyanVM, Assigned: mrbkap)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [pdfjs-c-integration])

Attachments

(1 file, 8 obsolete files)

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.
Blocks: e10s-tests
tracking-e10s: --- → +
Priority: -- → P2
Whiteboard: [pdfjs-c-integration]
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.
Attachment #8671112 - Flags: review?(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.
Attachment #8671112 - Flags: review?(mconley) → review+
Attachment #8671112 - Attachment is obsolete: true
Attachment #8672880 - Flags: review?(ydelendik)
Attachment #8672881 - Flags: review?(ydelendik)
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.
Attachment #8672882 - Flags: review?(ydelendik)
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.
Attachment #8672880 - Flags: review?(ydelendik) → review+
Comment on attachment 8672881 [details] [diff] [review]
Make browser_pdfjs_views.js e10s-compatible.

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

Looks good
Attachment #8672881 - Flags: review?(ydelendik) → review+
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.
Attachment #8672882 - Flags: review?(ydelendik) → review+
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.
Attachment #8673415 - Flags: review?(ydelendik)
Attachment #8672882 - Attachment is obsolete: true
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.
Attachment #8673415 - Flags: review?(ydelendik) → review+
Assignee: nobody → mrbkap
Backed out in a6f277178c7b for making the bug 1016737 leak-until-shutdown very very frequent on Windows.
Depends on: 1016737
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? :)
Flags: needinfo?(mrbkap)
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.
Depends on: 1222198
I've been working on other priorities but definitely will be getting back to this soon.
Attached patch Rebased patch (obsolete) — Splinter Review
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.
Attachment #8686907 - Flags: review?(ydelendik)
Attachment #8672879 - Attachment is obsolete: true
Attachment #8672880 - Attachment is obsolete: true
Attachment #8672881 - Attachment is obsolete: true
Attachment #8673415 - Attachment is obsolete: true
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).
Attachment #8686907 - Flags: review?(ydelendik) → review+
Depends on: 1224796
Depends on: 1224797
Depends on: 1224848
https://hg.mozilla.org/mozilla-central/rev/25a5db43969c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 45 → ---
Attached patch Fix intermittant oranges (obsolete) — Splinter Review
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.
Attachment #8688580 - Flags: review?(ydelendik)
Comment on attachment 8688580 [details] [diff] [review]
Fix intermittant oranges

Looks good. Let's try this method.
Attachment #8688580 - Flags: review?(ydelendik) → review+
Attached patch Rolled-up patchSplinter Review
Attachment #8688664 - Flags: review+
Attachment #8686907 - Attachment is obsolete: true
Attachment #8688580 - Attachment is obsolete: true
Flags: needinfo?(mrbkap)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ccb27d336e71
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Duplicate of this bug: 1224796
You need to log in before you can comment on or make changes to this bug.