Closed Bug 424377 Opened 14 years ago Closed 13 years ago

Timers shouldn't run while printing

Categories

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

x86
Windows Vista
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: smaug)

References

Details

(Keywords: crash, fixed1.9.1, testcase, Whiteboard: [sg:moderate] see also bug 425265. need to define "timeout fixes" (comments 66-72))

Attachments

(6 files, 3 obsolete files)

Attached file testcase
See testcase, when printing the testcase, the counter should stop increasing the number as soon as the print progress dialog comes up, because otherwise bugs like bug 424291 or bug 422294 might occur.
(this bug was in fact filed because bug 424291, comment 4 asked for it)
I think it's easy to get things to crash if this happens... We need to fix this, imo.  At the very least, we should not be running script while the print presentation is observing the DOM.
Group: security
Flags: blocking1.9?
Keywords: crash
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
I've been looking at this a bit. Taking.
Assignee: nobody → Olli.Pettay
Attached patch WIP (obsolete) — Splinter Review
Something like this. Fixes resuming timers too.
Needs still testing.
Attached file testcase 2
This creates a long page, so printing it takes some time.
Also added a dump to ease testing.
With the patch timers don't run while printing (or previewing) but are
resumed when coming back to the original view.
Also canceling printing resumes timers.

Martijn, can you think still something else to test?
Note to myself, shouldn't resume timers if 
nsGkAtoms::scriptEnabledBeforePrintPreview isn't set.
And change the name of nsGkAtoms::scriptEnabledBeforePrintPreview property.
Attached patch v2 (obsolete) — Splinter Review
Small changes - prevent re-resuming.
I tested this also with scripts disabled and seemed to work ok.

Martijn, would be great if you could test this on windows.

roc, don't know anyone else who is doing printing reviews :(
class nsScriptSuppressor is there to handle all the error cases.
Attachment #311406 - Attachment is obsolete: true
Attachment #311475 - Flags: review?(roc)
Attached file testcase3
Your v2 patch seems to stop scripts in iframes just fine, but it doesn't seem to reenable them.
Also, I would have thought the patch would fix bug 422294, but it doesn't seem to do that.
Comment on attachment 311475 [details] [diff] [review]
v2

Martijn, thanks for testing! At least I need to fix the iframe testcase.
Attachment #311475 - Flags: review?(roc)
Attached patch v3 (obsolete) — Splinter Review
This handles timer suspend/resume in a better way.
Attachment #311475 - Attachment is obsolete: true
Comment on attachment 311668 [details] [diff] [review]
v3

Jst, asking review because of timer suspend/resume changes.
(I hope roc could then sr)
Attachment #311668 - Flags: review?(jst)
(In reply to comment #12)
> (From update of attachment 311668 [details] [diff] [review])
> Jst, asking review because of timer suspend/resume changes.

I don't understand why you would ever *not* want to suspend timeouts in child windows here, i.e. why the need for the new argument to suspend/resume timeouts?
That is because printing code goes through all the subdocuments, or documents 
which it is going to print, and disables 
scripts and timers. In that case we don't want to suspend/resume subdocuments,
because printing code will do that in its loop - suspend/resume subdocuments
would lead to re-suspend/resume.
I could try to add some flag to globalwindow so that re-suspend/resume becomes
no-op.
The printing code starts at the top window when it goes through its windows right? If so, suspending on the top window would suspend all children, and a suspend call on a child window would be a no-op, same thing for resuming, resuming again would be a no-op too. And if we don't like that, it would seem better to make the printing code only do a suspend/resume on the top window (top of what's being printed, I guess) and not bother on child windows.

All of this is sort of bogus though as timers can still fire in other window objects that could have references to the window being printed, which would still lead to the same bugs. Less likely, but still very possible.
(In reply to comment #15)
> All of this is sort of bogus though as timers can still fire in other window
> objects that could have references to the window being printed, which would
> still lead to the same bugs. Less likely, but still very possible.

Ah, right. That is different, though equally bad, bug. Timers should be
suspended, because we don't want scripts to run, but how to prevent
other windows to access suspended windows :/

This should work too.
Not-suspending timers breaks things, so it should be done anyway, even if
it doesn't cause security problems.
I'll file a new bug about external windows accessing to-be-printed documents.
Attachment #311668 - Attachment is obsolete: true
Attachment #311876 - Flags: review?(jst)
Attachment #311668 - Flags: review?(jst)
Attachment #311876 - Flags: review?(jst) → review+
Attachment #311876 - Flags: superreview?(roc)
(In reply to comment #16)
> Ah, right. That is different, though equally bad, bug. Timers should be
> suspended, because we don't want scripts to run, but how to prevent
> other windows to access suspended windows :/

By cloning the entire document tree for printing, which will fix a lot of problems including that one.
+  // Enable scripting after printing.
+  scriptSuppressor.Disconnect();

This comment confuses me. We're explicitly NOT enabling scripting here, right? Should you say "We will enable scripting later after printing has finished"?
(In reply to comment #19) 
> This comment confuses me. We're explicitly NOT enabling scripting here, right?
> Should you say "We will enable scripting later after printing has finished"?
I should.

(In reply to comment #17)
> I'll file a new bug about external windows accessing to-be-printed documents.
Bug 425265

Comment on attachment 311876 [details] [diff] [review]
v4, add a flag to global window

I found one case where scripting is turned on too early. New patch coming, hopefully soon.
Attachment #311876 - Flags: superreview?(roc)
Whiteboard: [sg:critical?]
Comment on attachment 312225 [details] [diff] [review]
keep timers suspended if preview is shown still after printing

Argh, I forgot to ask sreview.
Attachment #312225 - Flags: superreview?(roc)
Attachment #312225 - Flags: superreview?(roc) → superreview+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008040204 Minefield/3.0pre
Testing the various testcase on print and print preview.
Status: RESOLVED → VERIFIED
Depends on: 427758
Version: unspecified → Trunk
Should we backout this. That would bring back the FF2 behavior, where
timers aren't suspended, but scripts are disabled.
So timers won't be resumed correctly after printpreview/print.
At least for RC1, to fix bug 427758?
Yes, if you think that's the right thing to do.
I'm still trying to find out some solution for bug 427758.
Comment on attachment 312225 [details] [diff] [review]
keep timers suspended if preview is shown still after printing

Asking approval to back this out to fix the regression :(
Attachment #312225 - Flags: approval1.9?
Comment on attachment 312225 [details] [diff] [review]
keep timers suspended if preview is shown still after printing

a1.9=beltzner
Attachment #312225 - Flags: approval1.9? → approval1.9+
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Keywords: checkin-needed
I'm not 100% sure this should block 1.9.
The patch didn't fix bug 422294 anyway.
The patch was backed out.
Fyi, I suspect the patch in this bug also caused the [@ nsPrintEngine::TurnScriptingOn] crash:
http://crash-stats.mozilla.com/report/list?range_unit=weeks&version=Firefox%3A3.0pre&range_value=2&signature=nsPrintEngine%3A%3ATurnScriptingOn(int)
Some comments in those reports:
"printed a popup from goodmortgage.com to a pdf"
"I was printing to PDF, and the browser crashed before it could finish."
"Printing pages to PDF from Consumer Reports web site. This seems to be a repeatable crash."
given comment 30 and 32 I think we should take this off the blocker list
Going ahead and taking this off blocker list.  Please re-nom if you disagree.
Flags: blocking1.9+ → blocking1.9-
Blocks: 431087
If you have questions if this security critical have a look at  430814 and the followup bug 431087. In my opinion this is certainly critical and opens a path to a whole class of nasty deleted memory or array boundary violation bugs. Having timers active during printing will violate core pagination principles at least for tables, for other generated content probably too.
This is something we really need to fix but we can't fix it at this stage of 1.9. Maybe we can fix it in 1.9.0.x. If we can find someone to implement the approach of cloning documents to be printed, that would also solve the problem.

One bit of good news here is that since it requires user interaction to print the page, or enter print preview, it doesn't make a good vector for automated attacks.
Whiteboard: [sg:critical?] → [missed 1.9 checkin][sg:critical?]
Comment on attachment 312225 [details] [diff] [review]
keep timers suspended if preview is shown still after printing

Removing approval since this missed the 1.9 cutoff.
Attachment #312225 - Flags: approval1.9+
Depends on: 428309
it would be cool to find resources for this, this causes pretty nasty crashes
Flags: wanted1.9.1?
Blocks: 422294
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1+
Blocks: 431242
Blocks: 459663
Flags: wanted1.9.0.x+
Smaug: are you still working on this?  It's currently marked blocking1.9.1+ with priority=P2, which I think technically means we'd hold the release on it.  (Though the priority field hasn't been touched since March 2008 -- perhaps it's no longer accurate & should be downgraded?)
I know this is blocking (and disturbing me). I've been just way too busy with
other things :(
Any help here would be appreciated, btw.
No longer blocks: 422294
Whiteboard: [missed 1.9 checkin][sg:critical?] → [sg:critical?]
Depends on: 425265
Whiteboard: [sg:critical?] → [sg:critical?], see also bug 425265
Looks like Smaug's patch has superreview.  It would be great to get this landed, though perhaps I'm misreading a dependency that is preventing us from doing so.
Whiteboard: [sg:critical?], see also bug 425265 → [sg:critical?], see also bug 425265, needs-landing
So I'd want bug 425265 to branch, not this one (fixing this wouldn't help with
crashes). So marking blocking1.9.1? in hope that this gets marked to blocking1.9.1- but bug 425265 gets blocking1.9.1+.
Flags: blocking1.9.1+ → blocking1.9.1?
Whiteboard: [sg:critical?], see also bug 425265, needs-landing → [sg:critical?], see also bug 425265
(In reply to comment #42)
> Looks like Smaug's patch has superreview.  It would be great to get this
> landed, though perhaps I'm misreading a dependency that is preventing us from
> doing so.
The patch here is not going to land, nor is this actually sg:critical,
or fixing this wouldn't fix any sg:critical bugs.

Fixing this bug would improve print preview usability though.
No longer blocks: 431087
No longer blocks: 431242
I'm about to update the patch for trunk.
Because of other timeout fixes it shouldn't regress bug 427758.
Not sure about bug 428309.
Attached patch v7Splinter Review
The changes to nsGlobalWindow/nsPIDOMWindow aren't needed anymore.
Added few null checks (to-not-regress bug 428309), otherwise the same patch as attachment 312225 [details] [diff] [review].
(In reply to comment #43)
> So I'd want bug 425265 to branch, not this one (fixing this wouldn't help with
> crashes). So marking blocking1.9.1? in hope that this gets marked to
> blocking1.9.1- but bug 425265 gets blocking1.9.1+.
So IMHO, this should not block, but be wanted1.9.1+.

The attached patch does improve usability quite a bit, because with it
pages like attachment 311410 [details] work after printing / print preview.
Attachment #366289 - Flags: superreview?(roc)
Attachment #366289 - Flags: review?(roc)
I've tested the patch also with sync XHR which also suspends timers.
So far looking good.
Uploaded the patch to tryserver https://build.mozilla.org/tryserver-builds/?C=M;O=D
The builds will be called something like opettay-ptimer
I'm having trouble reconciling Bernd's comment 33 (is critical) and Smaug's comment 43/44 (not critical). Did the sg:critical bits get moved into bug 425265 instead?
See the latter paragraph in comment 15.
And you probably mean Bernd's comment 35.
But sure, fixing this should decrease the risk for crashes.
No longer blocks: 459663
Attachment #366289 - Flags: superreview?(roc)
Attachment #366289 - Flags: superreview+
Attachment #366289 - Flags: review?(roc)
Attachment #366289 - Flags: review+
I tried the tryserver build, I haven't been able to crash with that build, thus far (tried for 30 minutes or so).
Not blocking, please renominate if you disagree. Will mark approved, though.
Flags: blocking1.9.1? → blocking1.9.1-
Crap - I approved that without asking for tests. Why weren't tests included?
I need to find some way to write tests.
I guess I'll ask mw22.
Attachment #366289 - Flags: approval1.9.1+ → approval1.9.1?
Comment on attachment 366289 [details] [diff] [review]
v7

Moving back to a191? until we get tests and it's had some time to bake ...
Tests are in bug 425265. I'll push them soon.
Status: REOPENED → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
Comment on attachment 366289 [details] [diff] [review]
v7

a191=beltzner, push it all at the same time (tests & fix)
Attachment #366289 - Flags: approval1.9.1? → approval1.9.1+
Backed out to see if it caused OSX orange (very unlikely).
Keywords: fixed1.9.1
It was not this bug which caused the orange.
Flags: blocking1.9.0.10?
To get this to 1.9.0.x requires porting all the timeout fixes we have on
trunk/1.9.1 to 1.9.0.x too.
Isn't that too risky?
Is "all the timeout fixes" defined or is that the kind of thing that will involve lots of bugzilla queries and risk of missing a bunch?
(In reply to comment #68)
> Is "all the timeout fixes" defined or is that the kind of thing that will
> involve lots of bugzilla queries and risk of missing a bunch?
All the fixes related to sync XHR and suspending timeouts.
So that answer is "involves bugzilla queries and risks missing some" then? At this point anything short of "needs bugs A,B,C..." is vague. Bug 425265 (which this requires) is already a 50K patch and presumably assumes the timeout fixes you're talking about.
Flags: blocking1.9.0.10?
Just to make it clear: We *do* want this on the 1.9.0 branch, but not for 1.9.0.10. I'd like to see a full list of bugs we're taking, which ones have testcases (automated or not), and a complete roll up patch of all of them, instead of an unknown amount of bugs with an unknown amount of risk.

Olli, can you start compiling that so we can consider this for 1.9.0.11? We'd want to take such a patch as soon as we open the tree for a release cycle.
I really hope this won't be taken on the 1.9.0 branch. One regression already occurred with this patch and the other patches and there might be lurking a few more. But I guess if Olli thinks it's safe, then I have no objection.
Flags: blocking1.9.0.12?
Whiteboard: [sg:critical?], see also bug 425265 → [sg:critical?] see also bug 425265. need to define "timeout fixes" (comments 66-72)
-> sg:moderate because user-action required.
Flags: blocking1.9.0.12? → blocking1.9.0.12-
Whiteboard: [sg:critical?] see also bug 425265. need to define "timeout fixes" (comments 66-72) → [sg:moderate] see also bug 425265. need to define "timeout fixes" (comments 66-72)
Group: core-security
You need to log in before you can comment on or make changes to this bug.