Closed
Bug 424377
Opened 16 years ago
Closed 15 years ago
Timers shouldn't run while printing
Categories
(Core :: Printing: Output, defect, P2)
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)
211 bytes,
text/html
|
Details | |
356 bytes,
text/html
|
Details | |
489 bytes,
text/html
|
Details | |
13.63 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
14.23 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
8.08 KB,
patch
|
roc
:
review+
roc
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•16 years ago
|
||
(this bug was in fact filed because bug 424291, comment 4 asked for it)
Comment 2•16 years ago
|
||
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.
Updated•16 years ago
|
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Assignee | ||
Comment 3•16 years ago
|
||
I've been looking at this a bit. Taking.
Assignee: nobody → Olli.Pettay
Assignee | ||
Comment 4•16 years ago
|
||
Something like this. Fixes resuming timers too. Needs still testing.
Assignee | ||
Comment 5•16 years ago
|
||
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?
Assignee | ||
Comment 6•16 years ago
|
||
Note to myself, shouldn't resume timers if nsGkAtoms::scriptEnabledBeforePrintPreview isn't set. And change the name of nsGkAtoms::scriptEnabledBeforePrintPreview property.
Assignee | ||
Comment 7•16 years ago
|
||
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)
Reporter | ||
Comment 8•16 years ago
|
||
Your v2 patch seems to stop scripts in iframes just fine, but it doesn't seem to reenable them.
Reporter | ||
Comment 9•16 years ago
|
||
Also, I would have thought the patch would fix bug 422294, but it doesn't seem to do that.
Assignee | ||
Comment 10•16 years ago
|
||
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)
Assignee | ||
Comment 11•16 years ago
|
||
This handles timer suspend/resume in a better way.
Attachment #311475 -
Attachment is obsolete: true
Assignee | ||
Comment 12•16 years ago
|
||
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)
Comment 13•16 years ago
|
||
(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?
Assignee | ||
Comment 14•16 years ago
|
||
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.
Comment 15•16 years ago
|
||
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.
Assignee | ||
Comment 16•16 years ago
|
||
(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 :/
Assignee | ||
Comment 17•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #311876 -
Flags: review?(jst) → review+
Assignee | ||
Updated•16 years ago
|
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"?
Assignee | ||
Comment 20•16 years ago
|
||
(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
Assignee | ||
Comment 21•16 years ago
|
||
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)
Assignee | ||
Comment 22•16 years ago
|
||
Updated•16 years ago
|
Whiteboard: [sg:critical?]
Assignee | ||
Comment 23•16 years ago
|
||
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+
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Updated•16 years ago
|
Flags: in-testsuite?
Reporter | ||
Comment 24•16 years ago
|
||
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
Updated•16 years ago
|
Version: unspecified → Trunk
Assignee | ||
Comment 25•16 years ago
|
||
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.
Assignee | ||
Comment 27•16 years ago
|
||
I'm still trying to find out some solution for bug 427758.
Assignee | ||
Comment 28•16 years ago
|
||
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 29•16 years ago
|
||
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+
Updated•16 years ago
|
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 30•16 years ago
|
||
I'm not 100% sure this should block 1.9. The patch didn't fix bug 422294 anyway.
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 31•16 years ago
|
||
The patch was backed out.
Reporter | ||
Comment 32•16 years ago
|
||
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."
Comment 33•16 years ago
|
||
given comment 30 and 32 I think we should take this off the blocker list
Comment 34•16 years ago
|
||
Going ahead and taking this off blocker list. Please re-nom if you disagree.
Flags: blocking1.9+ → blocking1.9-
Comment 35•16 years ago
|
||
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.
Updated•16 years ago
|
Whiteboard: [sg:critical?] → [missed 1.9 checkin][sg:critical?]
Comment 37•16 years ago
|
||
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+
Comment 38•16 years ago
|
||
it would be cool to find resources for this, this causes pretty nasty crashes
Flags: wanted1.9.1?
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1+
Updated•16 years ago
|
Flags: wanted1.9.0.x+
Comment 39•15 years ago
|
||
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?)
Assignee | ||
Comment 40•15 years ago
|
||
I know this is blocking (and disturbing me). I've been just way too busy with other things :(
Assignee | ||
Comment 41•15 years ago
|
||
Any help here would be appreciated, btw.
Updated•15 years ago
|
Whiteboard: [missed 1.9 checkin][sg:critical?] → [sg:critical?]
Assignee | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?], see also bug 425265
Comment 42•15 years ago
|
||
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
Assignee | ||
Comment 43•15 years ago
|
||
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?
Assignee | ||
Updated•15 years ago
|
Whiteboard: [sg:critical?], see also bug 425265, needs-landing → [sg:critical?], see also bug 425265
Assignee | ||
Comment 44•15 years ago
|
||
(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.
Assignee | ||
Comment 45•15 years ago
|
||
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.
Assignee | ||
Comment 46•15 years ago
|
||
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].
Assignee | ||
Comment 47•15 years ago
|
||
(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.
Assignee | ||
Updated•15 years ago
|
Attachment #366289 -
Flags: superreview?(roc)
Attachment #366289 -
Flags: review?(roc)
Assignee | ||
Comment 48•15 years ago
|
||
I've tested the patch also with sync XHR which also suspends timers. So far looking good.
Assignee | ||
Comment 49•15 years ago
|
||
Uploaded the patch to tryserver https://build.mozilla.org/tryserver-builds/?C=M;O=D The builds will be called something like opettay-ptimer
Comment 50•15 years ago
|
||
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?
Assignee | ||
Comment 51•15 years ago
|
||
See the latter paragraph in comment 15. And you probably mean Bernd's comment 35.
Assignee | ||
Comment 52•15 years ago
|
||
But sure, fixing this should decrease the risk for crashes.
Attachment #366289 -
Flags: superreview?(roc)
Attachment #366289 -
Flags: superreview+
Attachment #366289 -
Flags: review?(roc)
Attachment #366289 -
Flags: review+
Reporter | ||
Comment 53•15 years ago
|
||
I tried the tryserver build, I haven't been able to crash with that build, thus far (tried for 30 minutes or so).
Comment 54•15 years ago
|
||
Not blocking, please renominate if you disagree. Will mark approved, though.
Flags: blocking1.9.1? → blocking1.9.1-
Updated•15 years ago
|
Attachment #366289 -
Flags: approval1.9.1+
Comment 55•15 years ago
|
||
Comment on attachment 366289 [details] [diff] [review] v7 a191=beltzner
Comment 56•15 years ago
|
||
Crap - I approved that without asking for tests. Why weren't tests included?
Assignee | ||
Comment 57•15 years ago
|
||
I need to find some way to write tests. I guess I'll ask mw22.
Assignee | ||
Comment 58•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/bea12faeea80
Updated•15 years ago
|
Attachment #366289 -
Flags: approval1.9.1+ → approval1.9.1?
Comment 59•15 years ago
|
||
Comment on attachment 366289 [details] [diff] [review] v7 Moving back to a191? until we get tests and it's had some time to bake ...
Assignee | ||
Comment 60•15 years ago
|
||
Tests are in bug 425265. I'll push them soon.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → FIXED
Comment 61•15 years ago
|
||
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+
Assignee | ||
Comment 62•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f93a2f0091b3 I'll push the tests when landing Bug 425265.
Keywords: fixed1.9.1
Assignee | ||
Comment 63•15 years ago
|
||
Backed out to see if it caused OSX orange (very unlikely).
Keywords: fixed1.9.1
Assignee | ||
Comment 64•15 years ago
|
||
It was not this bug which caused the orange.
Assignee | ||
Comment 65•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/41b8e5e51ec7
Keywords: fixed1.9.1
Updated•15 years ago
|
Flags: blocking1.9.0.10?
Assignee | ||
Comment 66•15 years ago
|
||
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.
Reporter | ||
Comment 67•15 years ago
|
||
Isn't that too risky?
Comment 68•15 years ago
|
||
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?
Assignee | ||
Comment 69•15 years ago
|
||
(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.
Comment 70•15 years ago
|
||
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?
Comment 71•15 years ago
|
||
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.
Reporter | ||
Comment 72•15 years ago
|
||
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.
Updated•15 years ago
|
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)
Comment 73•15 years ago
|
||
-> 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)
Updated•11 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•