1627577 - Enable printing tests (access denied on tmp directory on print reftest on MacOSX)

Status: RESOLVED FIXED

(Core :: Printing: Output, defect, P1)

Description

[Hiroyuki Ikezoe (:hiro)]

When you run ./mach reftest layout/reftests/printing/test-text.html on MacOSX, you will see below error.

1:32.06 GECKO(1705) JavaScript error: resource://reftest/reftest-content.js, line 262: NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.createUnique]

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Is this a regression? This kind of writes was restricted ages ago, why is this test not failing on try?

Can we just...not do this from content?

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Raaah: https://bugzilla.mozilla.org/show_bug.cgi?id=1622093#c1

Comment 3

[Haik Aftandilian [:haik]]

We're trying to create a file /private/var/folders/<xx>/<...>/T/reftest-print.pdf in the content process which is blocked by process sandboxing. From the error, it's https://searchfox.org/mozilla-central/rev/a707541ff423ade0d81cef6488e6ecfa09273886/layout/tools/reftest/reftest-content.js#262

We shouldn't have to create the file from a content process. During the test, I checked the profile used and print_via_parent=false in the profile prefs. I'm not sure if I'm understanding the tests correctly, but it appears we need to rework the test to use remote printing and do the verification in the parent so that content processes don't need to read/write from the filesystem.

Glad this is being worked on. More test coverage of printing is greatly appreciated.

Comment 4

[Erik Nordin [:nordzilla]]

This is definitely reproducible on my macOS machine. I'm going to mark this as P3, given that we currently don't run these tests.

Comment 5

[Haik Aftandilian [:haik]]

(In reply to Haik Aftandilian [:haik] from comment #3)

We shouldn't have to create the file from a content process. During the test, I checked the profile used and print_via_parent=true in the profile prefs. I'm not sure if I'm understanding the tests correctly, but it appears we need to rework the test to use remote printing and do the verification in the parent so that content processes don't need to read/write from the filesystem.

To clarify what I meant by remote printing above, printing on Windows, Mac, and desktop Linux all use print.print_via_parent=true to enable remote printing which was added with bug 1156742 for Windows (which was a large project) and later enabled on Mac and desktop Linux. With remote printing, the print data is sent over IPC and the printing OS API calls are done from the parent process. This is done to allow stronger sandboxing in the content processes.

If we are testing with print_via_parent=false, 1) we will not be exercising large parts of our printing code paths that are used by desktop users 2) printing will not work due to sandboxing and we'll have to make sandboxing changes to allow it. For example, on Mac, content processes don't have access to OS printing services due to process-level sandboxing.

Comment 6

[Bob Owen (:bobowen)]

I have some patches that gets printing via the parent with these reftests working.
This along with some other changes to work-around the file system restrictions and other bits and pieces, means the tests work locally for me on Windows and macOS.
(Actually one of the tests failed on macOS, wrong number of pages, but perhaps that is a genuine issue.)

I'll get the patches up next week.
I suspect my fixes are not the best options in some circumstances and they include a patch to PDF.js, which looks like it'll have to be landed in a separate project (unless there is a better option that doesn't require changes).
So, I'll probably just attach the patches to this bug initially for feedback.

Comment 7

[Bob Owen (:bobowen)]

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

A few things to sort out, but looks like it could be enabled already on some configurations.

Comment 8

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&duplicate_jobs=visible&revision=739519c4fb0965bd7456daffd3fdf38ce168a17d

Lots of crashes on Linux/Android for some reason, so I disabled there:
https://treeherder.mozilla.org/#/jobs?repo=try&duplicate_jobs=visible&revision=c48263899ed1e904a972cefd7b85cce57cbf1e4c

One intermittent reading issue on mac, so we might have to disable there if it turns out to be too bad.

Comment 9

[Bob Owen (:bobowen)]

I'm just going to attach the patches to the bug for feedback initially, because if the PDF.js change is really needed then that will have to be landed on its repository I assume.

Comment 10

[Bob Owen (:bobowen)]

Attachment: Bug 1627577 part 1: Add function to allow window and document to be set in PDF.js module. r=bdahl!

Comment 11

[Bob Owen (:bobowen)]

Attachment: Bug 1627577 part 2: Change printing reftests to work with remote printing. r=emilio!

Comment 12

[Bob Owen (:bobowen)]

Attachment: Bug 1627577 part 3: Fix assertion in nsDeviceContextSpecWin::MakePrintTarget. r=jwatt!

Comment 13

[Bob Owen (:bobowen)]

Attachment: Bug 1627577 part 4: Don't get settings from printer when printing to PDF on macOS. r=jwatt!

Comment 14

[Bob Owen (:bobowen)]

Attachment: Bug 1627577 part 5: Enable printing restests on macOS and Windows. r=emilio!

Comment 15

[Bob Owen (:bobowen)]

Comment on attachment 9144283 [details] [diff] [review]
Bug 1627577 part 1: Add function to allow window and document to be set in PDF.js module. r=bdahl!

The getDocument function uses document to run the worker script and window to get the worker, but they are not set.
The global appears to be a Sandbox.

There might be a better way of doing this that doesn't require a change to PDF.js

bdahl - If this is acceptable I assume I have to land this on PDF.js repository.

Comment 16

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9144283 [details] [diff] [review]
Bug 1627577 part 1: Add function to allow window and document to be set in PDF.js module. r=bdahl!

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

This feels a bit hackish. Does something like this work instead?

```
with ({ window: g.containingWindow, document: g.containingWindow.document }) {
    // ...
}
```

Alternatively... Probably this is an artifact of using the devtools loader to require() pdf.js, but it seems that loading it as a `<script>` element should not have that sandboxing, so maybe either use a script [here](https://searchfox.org/mozilla-central/rev/2bfe3415fb3a2fba9b1c694bc0b376365e086927/layout/tools/reftest/reftest.xhtml#13) or lazily append it if not there when running a print reftest, and wait for its load event before continuing?

Comment 17

[Emilio Cobos Álvarez [:emilio]]

FYI, Linux / Android crashes are probably bug 1622075.

Comment 18

[Bob Owen (:bobowen)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #16)

Comment on attachment 9144283 [details] [diff] [review]
Bug 1627577 part 1: Add function to allow window and document to be set in
PDF.js module. r=bdahl!

Review of attachment 9144283 [details] [diff] [review]:

This feels a bit hackish. Does something like this work instead?

I thought that myself. :-)

with ({ window: g.containingWindow, document: g.containingWindow.document })
{
    // ...
}

Alternatively... Probably this is an artifact of using the devtools loader
to require() pdf.js, but it seems that loading it as a <script> element
should not have that sandboxing, so maybe either use a script
[here](https://searchfox.org/mozilla-central/rev/
2bfe3415fb3a2fba9b1c694bc0b376365e086927/layout/tools/reftest/reftest.
xhtml#13) or lazily append it if not there when running a print reftest, and
wait for its load event before continuing?

Thanks I'll give those a go.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

FYI, Linux / Android crashes are probably bug 1622075.

I think that crash is because it is printing in the content process, my changes should fix that as we're printing from the parent like in normal firefox.
One thought I had was whether we run the reftests in parallel on Linux that might be causing issues?

Comment 19

[Bob Owen (:bobowen)]

Comment on attachment 9144283 [details] [diff] [review]
Bug 1627577 part 1: Add function to allow window and document to be set in PDF.js module. r=bdahl!

I think I've got PDF.js loading lazily as a normal script.
I'm probably still committing crimes against JavaScript (if there are such things), but I'll get the patches up for review.

Comment 20

[Bob Owen (:bobowen)]

Attachment: Bug 1627577 part 1: Change printing reftests to work with remote printing. r=emilio!

Comment 21

[Bob Owen (:bobowen)]

Attachment: Bug 1627577 part 2: Fix assertion in nsDeviceContextSpecWin::MakePrintTarget. r=jwatt!

Depends on D73095

Comment 22

[Bob Owen (:bobowen)]

Attachment: Bug 1627577 part 3: Use NSPrintInfo for PrintRange, StartPageRange and EndPageRange in the parent on macOS. r=jwatt!

Depends on D73096

Comment 23

[Bob Owen (:bobowen)]

Attachment: Bug 1627577 part 4: Enable printing restests on macOS and Windows. r=emilio!

Depends on D73097

Comment 24

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&duplicate_jobs=visible&revision=4cb2a47ebeda8ea9e3a24303aad417271933eeb4

Comment 25

[Emilio Cobos Álvarez [:emilio]]

(In reply to Bob Owen (:bobowen) from comment #18)

I think that crash is because it is printing in the content process, my changes should fix that as we're printing from the parent like in normal firefox.
One thought I had was whether we run the reftests in parallel on Linux that might be causing issues?

I took a look, and it may be. They're not crashes per se, just timeouts. They all happen after a weird:

JavaScript error: , line 0: uncaught exception: Object

Which unfortunately is probably the worse JS error message ever. It'd be interesting if the new approach of loading the script fixes that or not. Otherwise please file a bug and ni? me, and I'm happy to try reproing here and poking at it with rr.

Thanks again for working on this!

Comment 26

[Jonathan Watt [:jwatt]]

Bob: dup bug 1434825?

Comment 28

[Bob Owen (:bobowen)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #25)

(In reply to Bob Owen (:bobowen) from comment #18)

I think that crash is because it is printing in the content process, my changes should fix that as we're printing from the parent like in normal firefox.
One thought I had was whether we run the reftests in parallel on Linux that might be causing issues?

I took a look, and it may be. They're not crashes per se, just timeouts. They all happen after a weird:

JavaScript error: , line 0: uncaught exception: Object

Which unfortunately is probably the worse JS error message ever.

By the way I got a similar error on Windows, which (if IIRC) I tracked down to the file.flush here [1], which seemed an odd thing to do for a read anyway.
So, it could be an issue opening/reading the file.
The tests currently use TmpD (which will be a separately created temporary dir in the content process, where it is called).
I wonder if this is sometimes causing problems, we could pass the filename down from the parent I guess.

[1] https://searchfox.org/mozilla-central/rev/7fd1c1c34923ece7ad8c822bee062dd0491d64dc/layout/tools/reftest/reftest.jsm#1763

Comment 29

[Bob Owen (:bobowen)]

Try push with new macOS print settings patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc28419c28d506dddb914955ebe45e421ac47726

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/103b671e8cb0
part 1: Change printing reftests to work with remote printing. r=emilio
https://hg.mozilla.org/integration/autoland/rev/763932c16c29
part 2: Fix assertion in nsDeviceContextSpecWin::MakePrintTarget. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/d868664d33b0
part 3: Use NSPrintInfo for PrintRange, StartPageRange and EndPageRange in the parent on macOS. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/fab960f9cf82
part 4: Enable printing restests on macOS and Windows. r=emilio

Comment 31

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/103b671e8cb0
https://hg.mozilla.org/mozilla-central/rev/763932c16c29
https://hg.mozilla.org/mozilla-central/rev/d868664d33b0
https://hg.mozilla.org/mozilla-central/rev/fab960f9cf82