1640533 - Audit whether MOZ_CAN_RUN_SCRIPT annotations for nsPrintJob functions are correct/necessary or not

Status: NEW

(Core :: Printing: Setup, task)

Description

[Hiroyuki Ikezoe (:hiro)]

For example, nsPrintJob::SetupToPrintContent has MOZ_CAN_RUN_SCRIPT, indeed it flushes pending styles/layout whatever, BUT in the function we flush the static-cloned documents which means there is no script in the documents unless I am misunderstanding what CreateStaticClone does.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

masayuki?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Some relevant questions;

• Window.print() should explicitly flush the document? (It seems not to me IIUC the spec), but..
• nsGlobalWindowInner::Print should be annotated as MOZ_CAN_RUN_SCRIPT since it fires beforeprint/afterprint events?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If the MOZ_CAN_RUN_SCRIPT is really safe, it may be fine to change it to MOZ_CAN_RUN_SCRIPT_BOUNDARY with comment explaining intentionally changed to it.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(but I don't know whether it's truly safe or not.)

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Thank you, masayuki. I am mostly 100% sure functions in nsPrintJob shouldn't run script at all because they are used for the static-cloned documents. One thing I am wondering is whether Document::CreateStaticClone should flush pending styles or not (currently it doesn't), and I suppose it shouldn't since users want to see results what the user is seeing at that moment.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Hmm nsPrintJob::TurnScriptingOn turns on/off scripts in the cloned documents. It's confusing to me. Do we really want to run scripts in the cloned documents?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Olli, at the time you introduced nsScriptSuppressor machinery in bug 424377 we hadn't cloned documents for printing, but we've cloned documents for printing since you fixed bug 487667, now we don't need the script turn on/off machinery at all? Am I understanding correctly?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Since it has been so long after those bugs, lots of the underlying code has changed possibly significantly, so one will need to check what all
https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/layout/printing/nsPrintJob.cpp#2843,2858 do.
And with Fission I assume some of the clone-doc-for-printing will need to become asynchronous, so disabling scripting temporarily will be probably needed - maybe.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Olli Pettay [:smaug] from comment #8)

Since it has been so long after those bugs, lots of the underlying code has changed possibly significantly, so one will need to check what all
https://searchfox.org/mozilla-central/rev/501eb4718d73870892d28f31a99b46f4783efaa0/layout/printing/nsPrintJob.cpp#2843,2858 do.

Thank you Olli, I will check the what Suspend() and Resume() do.

And with Fission I assume some of the clone-doc-for-printing will need to become asynchronous, so disabling scripting temporarily will be probably needed - maybe.

Yep, that's the initial motivation on this. (I am assuming you mean disabling scripting temporarily in original documents by the time we finished cloning all descendant documents).