Closed Bug 425265 Opened 12 years ago Closed 11 years ago

shouldn't allow any modifications to to-be-printed documents and/or presshells

Categories

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

x86
All
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Keywords: fixed1.9.1, Whiteboard: [sg:critical?])

Attachments

(5 files, 10 obsolete files)

3.66 KB, application/xhtml+xml
Details
v8
50.13 KB, patch
bzbarsky
: superreview+
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
7.59 KB, patch
Details | Diff | Splinter Review
2.39 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
51.10 KB, patch
Details | Diff | Splinter Review
See https://bugzilla.mozilla.org/show_bug.cgi?id=424377#c15

I'm not sure how to fix this one.
Flags: blocking1.9?
Group: security
Got an idea, very simple one. Will try it tomorrow.
(basically testing in cap if modifying document is allowed).
It may break something in those rare cases when pages which are being
printed are modified. But that is what we want to disable anyway.
Assignee: nobody → Olli.Pettay
Can you please give a bit more information on why you think this should block the release, and re-nom when you do so?
Flags: blocking1.9? → blocking1.9-
If this is not forbidden all sort of crash can occur, all in this case means nasty memory corruption.  Having fixed one of those bugs https://bugzilla.mozilla.org/show_bug.cgi?id=424291#c1 I am still impressed by martijn's hack. This is hacking at its very best. 
Another idea is to make the presshell of !IsDynamic precontext to
observe only changes to native anonymous content (== scrollbar).
Just an idea. Full page zoom crashes nicely with this.
I don't think we should try to fix this for 1.9. Fixing it properly is going to be very hard.

If some content is removed from the document, don't the frames associated with that content end up with dangling pointers?
(In reply to comment #6)
> I don't think we should try to fix this for 1.9. Fixing it properly is going 
> to be very hard.
I may agree with you.
 
> If some content is removed from the document, don't the frames associated with
> that content end up with dangling pointers?
Why? Presshell/FrameManager owns frames and frames own their content.

I also don't think we should try to fix this for 1.9.
I guess you're right Olli, but having live frames whose content is no longer in the document, maybe in another document, maybe with different children ... that scares me a lot.
Since bug 424377 is sg:critical? this one probably should be too.
Whiteboard: [sg:critical?]
Right. And this is what we should fix. Not the way my quick test patch does, but
some other way. Possibly by cloning the document.
Blocks: 424377
This is still using the same approach as attachment 312060 [details] [diff] [review],
and this works surprisingly well (fuzzer running in the background modifying the main presshell etc.).
A bit safer way could be to leave TurnScriptingOn() calls there.

Just disabling timers doesn't help with XHR etc.
Cloning doc might be the best option, but since we have the architecture for
multiple presshell's we could try to use it.
Attachment #312060 - Attachment is obsolete: true
Summary: shouldn't allow any modifications to to-be-printed documents → shouldn't allow any modifications to to-be-printed documents and/or presshells
Attached patch up-to-date (obsolete) — Splinter Review
The more I think this the more I like the approach.
A lot simpler than clone-doc approach, I guess; no need to clone doc, stylesheets etc.

The disable-timers approach wouldn't work too well, because XHR etc may still
run scripts.

For 1.9.1 we could take the patch without 
removing nsPrintEngine::TurnScriptingOn() and yet still fix some printing crashes. That would mean disabling print preview zoom (which doesn't work
too well anyway).

Roc, does that sound too evil to you?
Attachment #360131 - Attachment is obsolete: true
Letting the frame tree go completely wrong as the content tree changes underneath it still seems really scary to me.

I don't like breaking print preview scaling. Could we reconstruct the frame tree when we scale?

I still think we should clone the document, although this might be a good bulletproofing fix before and after we do that.
The changes to zooming handling fix also Bug 476841 and Bug 444448.

The patch does still contain TurnScriptingOn-removal. I'll make a patch
without that.
Attachment #360350 - Attachment is obsolete: true
Attached patch no changes to scripting handling (obsolete) — Splinter Review
Both patches fix also for example bug 431242.
And note, only full zoom works. Text zoom doesn't.
Text zoom would require full reflow, but full zoom can be handle in
nsSimplePageSequenceFrame.
Comment on attachment 360546 [details] [diff] [review]
no changes to scripting handling

I upload this to tryserver. The test build will be called 'pp4_ns'.
https://build.mozilla.org/tryserver-builds/?C=M;O=D
Awesome -- in that tryserver build, I've verified that bug 444448 and bug 476841 are both fixed. (as suggested in comment 15)
And btw, print preview zooming is also a lot faster with the patch.
Just try with HTML 5, ~680 pages.
(In reply to comment #14)
> I still think we should clone the document, although this might be a good
> bulletproofing fix before and after we do that.
roc, what do you think about the patch, especially for 1.9.1?
I think we should do this.

However, I don't see how your zooming support can actually work. Firing a resize reflow at the page sequence shouldn't work, since they don't support any kind of incremental reflow.
See the changes to nsSimplePageSequenceFrame. The patch makes non-initial
reflows to update the size based on PresContext()->GetPrintPreviewScale().
Currently PrintPreviewScale is used only for initial reflow.
Attachment #360546 - Flags: review?(roc)
What happens if someone changes the length of text node data? Normally nsTextFrame::CharacterDataChanged would fix up the frame's internal offsets, but that won't happen anymore.

Do we propagate notifications on "root native anonymous" content to make sure scrollbars keep working?
Comment on attachment 360546 [details] [diff] [review]
no changes to scripting handling

Bah, I need to do something to nsTextFrame::PaintText
Attachment #360546 - Flags: review?(roc)
Attached file a test case for text node change (obsolete) —
Attached image a test case for text node changes in SVG (obsolete) —
Attached patch patch, approach 5 (obsolete) — Splinter Review
This copies the text for printing, so sort-of on the way towards document cloning.

Comments about the evilness of the patch won't surprise me.
Attachment #360536 - Attachment is obsolete: true
Attachment #360546 - Attachment is obsolete: true
It's evil :-). I would hate to add complexity to nsTextFrame for a temporary fix like this.

How about forwarding CharacterDataChanged notifications to the frame tree, and just clearing out mContent if that happens in a non-dynamic presentation? Then add a few null checks where necessary, hopefully only Paint. Just bailing out is fine, I don't care if we fail to paint text because someone is doing something evil.
Yeah, that could be one option. I even tried it, or something similar, where
nsTextFrame with invalid data isn't painted.
http://java.sun.com/javase/6/docs/api/ is a good testcase for
print preview zoom performance.
(In reply to comment #32)
> How about forwarding CharacterDataChanged notifications to the frame tree, and
> just clearing out mContent if that happens in a non-dynamic presentation?
Not clearing mContent, but clearing mTextrun and not re-creating it almost 
works.
But FirstLetterFrames cause pretty big trouble :(
Hmm, I need to check if not-deleting FirstLetterFrames would be ok
(In reply to comment #36)
> Hmm, I need to check if not-deleting FirstLetterFrames would be ok
Can't work the way I want.
Does the patch in comment #31 handle first-letter problems?

What happens if you change the 'type' attribute of an <input>?

Allowing the DOM to change without changing the frame tree at all just scares me a lot.
Attached file test case (obsolete) —
Approach 5 has no problems with things like this.
Tested with DEBUG_smaug and print.debug.scripting_on = true
(In reply to comment #38)
> Allowing the DOM to change without changing the frame tree at all just scares
> me a lot.
But for 1.9.1 I can't think any other way to make printing less crash-prone.
Bug 424377 clearly wouldn't help at all, but that is still marked blocking1.9.1+.

I did some more testing with "approach 5". Changing iframe urls, and
adding and removing iframes works fine, based on code inspection mathml
is safe etc.
If we accept this patch for 1.9.1 will you work on document cloning for 1.9.2? Or is this going to be an ugly temporary fix that lives forever?
We have a bit different opinions here. I like the concept of having static 
nsIFrame tree, but I don't like nsIFrames to depend on their mContent's data.
That data should be used only when creating the layout object, or when
explicitly changing mContent's state, IMO.
But I'm not a layout hacker.
Actually, or eventually, the static nsIFrame tree could be combined with cloned 
document approach.

There is also the option to fix non-galley presentation to support dynamic
changes. Though, changing print preview dynamically might be pretty odd.

For 1.9.2 I could think of working on an approach where
static nsIFrame tree is created using the original DOM, and then
each nsIContent, which the layout depends on, is cloned and added to (not-deep) 
cloned document and presshell would point to that cloned document.
So the cloned document would be a subset of the original document.
(In reply to comment #42)
> We have a bit different opinions here. I like the concept of having static 
> nsIFrame tree, but I don't like nsIFrames to depend on their mContent's data.
> That data should be used only when creating the layout object, or when
> explicitly changing mContent's state, IMO.

I'm not sure if this is something we really want. We certainly can't process style changes independently of the document. We could probably allow reflow and painting with some work, maybe a lot of work. But I don't know what that's useful for other than to make printing more robust here.

To tell the truth, I have got a crazy idea about making *display lists* be able to paint independently from the document and the frame tree, on another thread even, so that we can paint off the main thread and do parallel painting. But I want to be able to paint in parallel with layout/style changes/script execution, so for that job the frame tree/content boundary isn't the right place.

> There is also the option to fix non-galley presentation to support dynamic
> changes. Though, changing print preview dynamically might be pretty odd.

That's just the "Page Layout" view that a lot of word processors have. It would be nice to support.

> For 1.9.2 I could think of working on an approach where
> static nsIFrame tree is created using the original DOM, and then
> each nsIContent, which the layout depends on, is cloned and added to (not-deep) 
> cloned document and presshell would point to that cloned document.
> So the cloned document would be a subset of the original document.

Cloning the document would let us have a one-to-one mapping between presentations and documents. We could get rid of the primary frame map and just let elements have a pointer to their frames. Instead of GetPresShell(0) bogusness we could be honest and just have nsIDocument::GetPresShell/GetPresContext. I really want that.
(In reply to comment #43)
> Cloning the document would let us have a one-to-one mapping between
> presentations and documents. We could get rid of the primary frame map and just
> let elements have a pointer to their frames. Instead of GetPresShell(0)
> bogusness we could be honest and just have
> nsIDocument::GetPresShell/GetPresContext. I really want that.
Right. That the cloning would do, but I was thinking to create the frame
tree using the original document, and clone only the necessary parts of the
document, and the make nsIFrame::mContent to point to the clone.
(In reply to comment #44)
> and the make nsIFrame::mContent to point to the clone.
and then ...
Btw, seems like printing isn't handle properly in any browser engine currently.
Safari hangs easily, Chrome crashes, IE seems to hang the whole OS occasionally, 
Opera crashes, and Gecko crashes.
(In reply to comment #43)
> We could get rid of the primary frame map and just
> let elements have a pointer to their frames.
Btw, that is something I've never liked in webkit.
It doesn't allow (even in theory) to create several presentations of the
same DOM and it wastes memory in data documents.
I can't see that multiple presentations of the same document is ever going to be important. We can use canvas or some kind of <portal> element to make sections of one document render somewhere else, even pass through events, and we can apply transforms to those other views too. This would be enough to implement an editor with "split panes" or multiple windows onto the same document, for example. For static documents we can clone the document. I don't know what the use cases are where you need completely different layouts of a single document, at the same time, which stay in sync during dynamic changes to the content.

Maybe I'm wrong, but I guess data documents are less common than actual rendered documents, and not needing the primary frame map would actually reduce memory usage (as well as improve performance and simplify code).
Having said that, I'm coming around to taking your patch for 1.9.1, although I have a few comments. But I'd like to hear what Boris and David have to say about the big issues.
BTW true multiple presentations interacts with XBL in difficult ways, because you have to have multiple shadow trees. That may be solvable but it would be painful.
(In reply to comment #43)
> (In reply to comment #42)
> > We have a bit different opinions here. I like the concept of having static 
> > nsIFrame tree, but I don't like nsIFrames to depend on their mContent's data.
> > That data should be used only when creating the layout object, or when
> > explicitly changing mContent's state, IMO.
> 
> I'm not sure if this is something we really want. We certainly can't process
> style changes independently of the document. We could probably allow reflow and
> painting with some work, maybe a lot of work.
Actually, if nsIFrames wouldn't depend on nsIContent, moving reflow and
paint to a separate thread might become easier.
Reflowing on a separate thread is less interesting because script so often needs to do synchronous reflow flushes.
Also, we're close to having interruptible reflow for non-flush reflows.
Also, I've got other ideas for improving inline layout so that we don't need to create lots of of span frames and other goop, and they depend on walking the DOM more.

Also, making frames reflow and paint independently of content requires a TON of work. For example, MathML and XUL layout, and SVG rendering, looks directly at lots of DOM attributes.
For what it's worth, if we could store frame pointers in elements that would simplify a whole bunch of code, I suspect.  And no matter which approach we take there, I think cloning the whole document for printing is the way to go.
Attachment #361524 - Flags: review?(roc)
+  PRBool IsInRootNativeAnonymous(nsIContent* aContent) {
+    return aContent && aContent->IsInDoc() &&
+           aContent->IsInNativeAnonymousSubtree() &&
+           aContent->FindFirstNonNativeAnonymous() ==
+             aContent->GetOwnerDoc()->GetRootContent();
+  }

Hmm, can this return true for :before/:after content of the root element, and cause trouble for you that way?

+const nsTextFragment*
+nsTextFrame::GetFragment() const
+{
+  return PresContext()->IsDynamic() ? mContent->GetText() :
+    static_cast<const nsTextFragment*>(PresContext()->PropertyTable()->
+                                         GetProperty(mContent, nsGkAtoms::clonedTextForPrint));
+}

This looks like overhead I'd rather not have. Unfortunately we're out of frame state bits, but you could rename TEXT_BLINK_ON to TEXT_BLINK_ON_OR_PRINTING and set it when the text is cloned, then make GetFragment inlineable.

+  nsresult rv = nsTextFrame::InitTextFrame(PresContext(), aContent);

It doesn't make sense to call this from SVG, so how about making it nsContentUtils::CloneTextIfNeeded?

I'd like Boris to review this as well, since this is so wild and crazy.
Attachment #361515 - Attachment is obsolete: true
Attachment #361516 - Attachment is obsolete: true
Attachment #362136 - Attachment is obsolete: true
Attached patch v7 (obsolete) — Splinter Review
Attachment #361524 - Attachment is obsolete: true
Attachment #363161 - Flags: review?(roc)
Attachment #361524 - Flags: review?(roc)
Comment on attachment 363161 [details] [diff] [review]
v7

<grits teeth>
Attachment #363161 - Flags: superreview?(bzbarsky)
Attachment #363161 - Flags: review?(roc)
Attachment #363161 - Flags: review+
Awesome, looks like we have a reviewed patch \o/  This will be a great one to get landed and this is also the last dependency of bug 424377.  Thanks, Smaug.
Whiteboard: [sg:critical?] → [sg:critical?] needs-landing
>+++ b/layout/base/nsDocumentViewer.cpp
> DocumentViewerImpl::SetFullZoom(float aFullZoom)
>+#ifdef NS_PRINT_PREVIEW

So how come descendant prescontexts don't need to have this zoom set?

>+      shell->FrameNeedsReflow(f, nsIPresShell::eResize, NS_FRAME_IS_DIRTY);

So this only works because it triggers pagesequence to change its size without reflowing anything inside it, right?  So it's closer to an image viewer zoom than our normal full zoom?

>+++ b/layout/base/nsLayoutUtils.h
>+   * Initilizes text run container for printing. Does nothing if aPresContext
>+   * is dynamic.
>+   */
>+  static nsresult InitTextRunContainerForPrinting(nsPresContext* aPresContext,

This needs better documentation.  Why does it need aPresContext if it's got aFrame?  What will it do if aContainerContent is not text content?  Where do aBits come in?  How are aFrame and aContainerContent related?  Is any kind of aFrame alowed?  Assert whatever relationships are assumed here.

>+++ b/layout/base/nsPresShell.cpp
>+  nsDocumentObserverForNonDynamicPresContext(nsIDocumentObserver* aBaseObserver)
>+  : mBaseObserver(aBaseObserver) {}

Assert that aBaseObserver is non-null, please.

>+  PRBool IsInRootScrollbar(nsIContent* aContent) {
>+    if(aContent && aContent->IsInDoc()) {
>+       nsIContent* root = aContent->GetOwnerDoc()->GetRootContent();

That should be GetCurrentDoc(), no?

>+           if (tag == nsGkAtoms::scrollbar || tag == nsGkAtoms::scrollcorner) {
>+             return PR_TRUE;
>+           }
>+           break;

How about just:

  return tag == nsGkAtoms::scrollbar || tag == nsGkAtoms::scrollcorner;

?  

>+++ b/layout/printing/nsPrintEngine.cpp
>+#ifdef DEBUG_smaug
>+  if (nsContentUtils::GetBoolPref("print.debug.scripting_on", PR_FALSE)) {
>+    return;
>+  }
>+#endif

I'm not sure I follow this change.

So this patch overall is assuming that the only display items that touch their content node in a trivial way are the text-painting ones, hence the cloning of the text fragments?  I assume we've carefully checked this assumption?
(In reply to comment #61)
> So how come descendant prescontexts don't need to have this zoom set?
nsSimplePageSequenceFrame does the scaling.

> So this only works because it triggers pagesequence to change its size without
> reflowing anything inside it, right?  So it's closer to an image viewer zoom
> than our normal full zoom?
Right. And that is the behavior we want for print preview zoom, IMO.
This makes print preview scaling *very* fast.
Btw, this way we could fix the scroll bar size bug with non-native-style-themes.
Just set the right print preview scale and zoom, or something close to that. 

> This needs better documentation.  Why does it need aPresContext if it's got
> aFrame?  What will it do if aContainerContent is not text content?  Where do
> aBits come in?  How are aFrame and aContainerContent related?  Is any kind of
> aFrame alowed?  Assert whatever relationships are assumed here.
OK
 
> Assert that aBaseObserver is non-null, please.
OK.

> That should be GetCurrentDoc(), no?
Indeed.


>   return tag == nsGkAtoms::scrollbar || tag == nsGkAtoms::scrollcorner;
OK.
 
> >+++ b/layout/printing/nsPrintEngine.cpp
> >+#ifdef DEBUG_smaug
> >+  if (nsContentUtils::GetBoolPref("print.debug.scripting_on", PR_FALSE)) {
> >+    return;
> >+  }
> >+#endif
> 
> I'm not sure I follow this change.
I'll remove that. It is just for my testing. Normally we always disable scripts
when printing / print previewing. In my debug builds I set that pref to true and
scripts run all the time while printing or print previewing.
That way I can easily test what happens when primary presshell is modified.
I've tested using my own testcases and also some fuzzer while print previewing. 

> So this patch overall is assuming that the only display items that touch their
> content node in a trivial way are the text-painting ones, hence the cloning of
> the text fragments?  I assume we've carefully checked this assumption?
I've tried to do that yes. And that is why I've run all sort of evil tests
while print previewing.
Whiteboard: [sg:critical?] needs-landing → [sg:critical?]
Attached patch v8Splinter Review
Attachment #363161 - Attachment is obsolete: true
Attachment #366100 - Flags: superreview?(bzbarsky)
Attachment #363161 - Flags: superreview?(bzbarsky)
Attachment #366100 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 366100 [details] [diff] [review]
v8

Let's keep fingers crossed!
Asking blocking1.9.1.
See https://bugzilla.mozilla.org/show_bug.cgi?id=424377#c43
Flags: blocking1.9.1?
http://hg.mozilla.org/mozilla-central/rev/0e8572605937
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 444448
Blocks: 476841
Blocks: 431087
Blocks: 431242
Flags: wanted1.9.0.x+
See trivial followup patch on bug 482379, to fix a build failure when building with "--disable-svg". (Make sure to land that as well, if/when this lands on 1.9.1 and 1.9.0.x)
Same as bug 424377.
Flags: blocking1.9.1? → blocking1.9.1-
Attachment #366100 - Flags: approval1.9.1?
Comment on attachment 366100 [details] [diff] [review]
v8

This needs to land on 1.9.1 early to flush out regressions.
Ah, I thought I couldn't write automatic tests, but perhaps I can.
nsCanvasRenderingContext2D::DrawWindow uses the current preshell, not
primary shell.
Flags: in-testsuite?
Based on the tests for bug 396024.
Actually, how should we handle test cases for security bugs.
Perhaps better to push the tests after fixing this and bug 424377 on branch.
Depends on: 482976
Comment on attachment 366100 [details] [diff] [review]
v8

a191=beltzner (push the tests and the fix all at the same time)
Attachment #366100 - Flags: approval1.9.1? → approval1.9.1+
No longer depends on: 482976
Re-adding the dependency I just removed. (Yay bugzilla and form value retention!)

Are we getting this on 191?
Depends on: 482976
Yes. I'm hoping to get approval for Bug 482976, so that I don't land a 
patch (this bug) which is known to cause crashes.
Argh, I just noticed that nsSVGGlyphFrame::Init is inside #ifdef DEBUG in
trunk, and that method doesn't exist at all in 1.9.1.
Will upload a followup for trunk and updated patch for 1.9.1.
This moves #ifdef DEBUG
Attachment #369762 - Flags: superreview+
Attachment #369762 - Flags: review+
Attached patch for 1.9.1Splinter Review
Keywords: fixed1.9.1
Flags: in-testsuite? → in-testsuite+
Is this something we could get on the 1.9.0 branch without too much trouble?
Flags: blocking1.9.0.10?
Blocking assuming the answer looks like "yes"
Flags: blocking1.9.0.10? → blocking1.9.0.10+
We shouldn't take this on the branch until we're ready to take the whole group
Flags: blocking1.9.0.10+
Depends on: 489224
Flags: blocking1.9.0.12?
Comment 6 through 8 and a 50K patch with regressions say we shouldn't take this on 1.9.0 unless forced to by a firedrill (and even then look for a localized band-aide).
Flags: blocking1.9.0.12? → blocking1.9.0.12-
Group: core-security
You need to log in before you can comment on or make changes to this bug.