1705327 - Remove infobar notification from pdf viewer

Status: VERIFIED FIXED

(Firefox :: PDF Viewer, task)

Description

[Calixte Denizet (:calixte)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1705060#c2:

UX recommends not showing this infobar notification unless there is a significant security reason to include it

Comment 1

[Calixte Denizet (:calixte)]

Attachment: Bug 1705327 - Remove infobar notification from pdf viewer

Recommandation from UX is to not show this bar, so remove it.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a802bfb09181
Remove infobar notification from pdf viewer r=bdahl

Comment 3

[Natalia Csoregi [:nataliaCs]]

Backed out for failures on browser_pdfjs_notification_close_on_navigation.js

backout: https://hg.mozilla.org/integration/autoland/rev/b0887ad1341fa9e85b7bfd4bcf007cc912937aac

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=a802bfb0918178fc4599cf39de344ce3c4648ae3&selectedTaskRun=IO-hloKBQBWZD0Uu1NAy6g.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=336718237&repo=autoland&lineNumber=2707

[task 2021-04-15T22:53:52.327Z] 22:53:52 INFO - TEST-INFO | screentopng: exit 0
[task 2021-04-15T22:53:52.328Z] 22:53:52 INFO - Buffered messages logged at 22:53:44
[task 2021-04-15T22:53:52.329Z] 22:53:52 INFO - Entering test bound test_notification_is_removed_upon_navigation
[task 2021-04-15T22:53:52.332Z] 22:53:52 INFO - Buffered messages finished
[task 2021-04-15T22:53:52.333Z] 22:53:52 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/pdfjs/test/browser_pdfjs_notification_close_on_navigation.js | Uncaught exception - waiting for notification - timed out after 50 tries.
[task 2021-04-15T22:53:52.334Z] 22:53:52 INFO - Leaving test bound test_notification_is_removed_upon_navigation
[task 2021-04-15T22:53:52.336Z] 22:53:52 INFO - GECKO(1534) | MEMORY STAT | vsize 3127MB | residentFast 334MB | heapAllocated 91MB
[task 2021-04-15T22:53:52.337Z] 22:53:52 INFO - TEST-OK | toolkit/components/pdfjs/test/browser_pdfjs_notification_close_on_navigation.js | took 7733ms

Comment 4

[Natalia Csoregi [:nataliaCs]]

Also failing: TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | testE: selected text - got "", expected "aa\nbbbb\nee"

[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - TEST-PASS | dom/base/test/test_user_select.html | testB: range[0].endOffset
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - Buffered messages finished
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | testE: selected text - got "", expected "aa\nbbbb\nee"
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - checkText@dom/base/test/test_user_select.html:108:7
[task 2021-04-15T23:31:24.790Z] 23:31:24 INFO - test@dom/base/test/test_user_select.html:242:12
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | testE: Selection range count - got +0, expected 3
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - checkRangeCount@dom/base/test/test_user_select.html:127:7
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - test@dom/base/test/test_user_select.html:243:18
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | uncaught exception - IndexSizeError: Selection.getRangeAt: 0 is out of range at checkRange@http://mochi.test:8888/tests/dom/base/test/test_user_select.html:132:17
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - test@http://mochi.test:8888/tests/dom/base/test/test_user_select.html:244:13
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO -
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - simpletestOnerror@SimpleTest/SimpleTest.js:2112:18
[task 2021-04-15T23:31:24.791Z] 23:31:24 INFO - GECKO(1553) | JavaScript error: http://mochi.test:8888/tests/dom/base/test/test_user_select.html, line 132: IndexSizeError: Selection.getRangeAt: 0 is out of range
[task 2021-04-15T23:31:24.792Z] 23:31:24 INFO - GECKO(1553) | MEMORY STAT | vsize 20974971MB | residentFast 1350MB
[task 2021-04-15T23:31:24.792Z] 23:31:24 INFO - TEST-OK | dom/base/test/test_user_select.html | took 507ms

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0851da1d6c1d
Remove infobar notification from pdf viewer r=bdahl

Comment 6

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest plain failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/120aba36a06e7ae6dbd608e7791b139f25f01878

Push with failures

Failure log

INFO - Buffered messages finished
[task 2021-04-16T08:46:31.750Z] 08:46:31 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | testE: selected text - got "", expected "aa\nbbbb\nee"
[task 2021-04-16T08:46:31.750Z] 08:46:31 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2021-04-16T08:46:31.751Z] 08:46:31 INFO - checkText@dom/base/test/test_user_select.html:108:7
[task 2021-04-16T08:46:31.751Z] 08:46:31 INFO - test@dom/base/test/test_user_select.html:242:12
[task 2021-04-16T08:46:31.752Z] 08:46:31 INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-04-16T08:46:31.753Z] 08:46:31 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | testE: Selection range count - got +0, expected 3
[task 2021-04-16T08:46:31.754Z] 08:46:31 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:500:14
[task 2021-04-16T08:46:31.754Z] 08:46:31 INFO - checkRangeCount@dom/base/test/test_user_select.html:127:7
[task 2021-04-16T08:46:31.755Z] 08:46:31 INFO - test@dom/base/test/test_user_select.html:243:18
[task 2021-04-16T08:46:31.755Z] 08:46:31 INFO - Not taking screenshot here: see the one that was previously logged
[task 2021-04-16T08:46:31.756Z] 08:46:31 INFO - TEST-UNEXPECTED-FAIL | dom/base/test/test_user_select.html | uncaught exception - IndexSizeError: Selection.getRangeAt: 0 is out of range at checkRange@http://mochi.test:8888/tests/dom/base/test/test_user_select.html:132:17
[task 2021-04-16T08:46:31.756Z] 08:46:31 INFO - test@http://mochi.test:8888/tests/dom/base/test/test_user_select.html:244:13

Comment 7

[Calixte Denizet (:calixte)]

:sandor, I don't think this failure is related to the patch I landed.
When I open log and then I open the job for this log I get:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=089fd9e33fe26d5b4215692f84493b2026a071f2&selectedTaskRun=GMMNMWOlQAOVLTGhebnXYg.0

I can see this failing job:
https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=AGz6wbbxRfm3ynvhBgtYug.0
but its name 16-089fd9e33fe2-bk seems to indicate again an issue with the above patch.

Could you fix that please ?

Comment 8

[Sandor Molnar[:smolnar]]

That log is from where we discovered the failures.

You can see in this range,that the test under your patch are green and they are starting to fail after your patch

https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunning%2Cpending%2Crunnable&fromchange=a00cdb29df1f9c9939332094c15c89026f31eb66&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Ctest-linux1804-64%2Fdebug-mochitest-plain-e10s%2C16&tochange=089fd9e33fe26d5b4215692f84493b2026a071f2&group_state=expanded&selectedTaskRun=AGz6wbbxRfm3ynvhBgtYug.0

The name is from the backfilling action task. it shows from what change set the backfill started.
You can read about this feature here

Comment 9

[Calixte Denizet (:calixte)]

:sandor, ok I understand the backout.
I'm pretty sure that this patch is not the real culprit since it just aims to remove an infobar from pdf reviewer and there nothing related to text selection here.

Comment 10

[Calixte Denizet (:calixte)]

So there is something wrong with tests here:
https://searchfox.org/mozilla-central/source/dom/base/test/mochitest.ini

I just disabled the two tests related to pdf and I got the same error:
https://treeherder.mozilla.org/jobs?repo=try&revision=49d1a46d59b564305924db02204f038f53f673d7

So my patch is not the root cause of the regression but just highlights something wrong.
This test: https://searchfox.org/mozilla-central/source/dom/base/test/test_pdf_print.html
loads a bad pdf and so two (??) infobars are displayed.
So my patch implies that these infobars aren't displayed anymore (the innerHeight property isn't the same).
Since I can reproduce locally, I made a try in keeping only pdf tests and test_user_select in the mochitest.ini and of course everything runs fine !? so something else is wrong.
Using luck and dichotomy, I managed to get similars errors with test_user_select with this basic mochitest.ini:

[test_meta_viewport_fixed_width_and_zero_display_width.html]
[test_user_select.html]

:botond, would you have an idea here ?

Comment 11

[Botond Ballo [:botond]]

I can reproduce the failure with those two tests run in sequence, and it certainly looks weird visually (as if the whole harness page briefly zooms in, although I'm not seeing an actual change to the resolution that would result in that).

I will try to get to the bottom of it, but if you need a quick fix in the meantime, moving the test_meta_viewport_* tests into their own subdirectory should help.

Comment 12

[Botond Ballo [:botond]]

Investigation so far shows that, at the beginning of test_user_select.html, the top-level content window (the one containing the mochitest harness) has a nonzero scroll position. That scroll seems to come from here. Not sure yet why the previous test affects whether or not that happens.

Comment 13

[Botond Ballo [:botond]]

Seems to be related to test_meta_viewport_fixed_with_and_zero_display_width.html changing layout.css.devPixelsPerPx via scaleRatio() here. This does cause the whole test harness to zoom in, albeit using full-zoom rather than resolution.

Comment 14

[Botond Ballo [:botond]]

Ok, so the logical fix seems to be to pair this call to pushPrefEnv() (which changes layout.css.devPixelsPerPx) with a corresponding call to popPrefEnv() at the end of the test.

However, this does not seem to have the desired effect. While the device pixel ratio is changed back to 60, and we get e.g. a call to AppUnitsPerDevPixelChanged() for it, the visible area of the root content document's pres shell remains stuck at a size corresponding to the old ratio, and while the root content doc does get reflowed, it's reflowed into this stale size.

I'm not sure what is supposed to trigger the updating of the pres shell's visible area and a reflow to the correct size after the device pixel ratio changes. cc'ing Emilio in case he has some ideas.

Perhaps changing layout.css.devPixelsPerPx, which is an action that affects the mochitest harness document itself, is not a well-behaved thing for a mochitest to be doing in the first place?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45376b7c2ff5
Remove infobar notification from pdf viewer r=bdahl

Comment 16

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/45376b7c2ff5

Comment 17

[Calixte Denizet (:calixte)]

:RT, do you want to have this patch in 89 ?

Comment 18

[Romain Testard [:RT]]

(In reply to Calixte Denizet (:calixte) from comment #17)

:RT, do you want to have this patch in 89 ?

Yes please, the infobar shows currently on 89, which is counter to our proton goals so can we please consider this as part of the proton uplifts?

Comment 19

[Calixte Denizet (:calixte)]

Comment on attachment 9216037 [details]
Bug 1705327 - Remove infobar notification from pdf viewer

Beta/Release Uplift Approval Request

• User impact if declined: User will continue to see the infobar when opening some specific pdfs.
• 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: Bug 1706021
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Because it just removes an infobar.
• String changes made/needed:

Comment 20

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9216037 [details]
Bug 1705327 - Remove infobar notification from pdf viewer

Looks low risk, has tests and baked a few days on nightly, approved for 89 beta 5, thanks.

Comment 21

[Pascal Chevrel:pascalc (PTO until April 26)]

https://hg.mozilla.org/releases/mozilla-beta/rev/120a5201ab31

Comment 22

[Mark Striemer [:mstriemer]]

Looks like this was a dupe of bug 1701006, copying its proton flags over

Comment 24

[Simona Badau, Desktop QA]

Verified as fixed using the latest Nightly 90.0a1 and the latest Firefox 89 beta 9 - the PDF Infobar notification has been removed. Verified on macOS BigSur 11, Ubuntu 20.04, and Windows 10.