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

Status: RESOLVED FIXED

(Core :: Printing: Output, defect)

Description

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=424377#c15

I'm not sure how to fix this one.

Comment 1

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

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.

Comment 2

[Damon Sicore (:damons)]

Can you please give a bit more information on why you think this should block the release, and re-nom when you do so?

Comment 3

[Bernd]

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. 

Comment 4

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

Another idea is to make the presshell of !IsDynamic precontext to
observe only changes to native anonymous content (== scrollbar).

Comment 5

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

Attachment: quick test patch for 'disconnected print/preview presshell'

Just an idea. Full page zoom crashes nicely with this.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 7

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

(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.

Comment 8

[Johnny Stenback (:jst)]

I also don't think we should try to fix this for 1.9.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 10

[Brandon Sterne (:bsterne)]

Since bug 424377 is sg:critical? this one probably should be too.

Comment 11

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

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.

Comment 12

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

Attachment: DocumentObserver wrapper for print presshell

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.

Comment 13

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

Attachment: up-to-date

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?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

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

Attachment: +support for full zoom in print preview

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.

Comment 16

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

Attachment: no changes to scripting handling

Comment 17

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

Both patches fix also for example bug 431242.

Comment 18

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

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 19

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

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

Comment 20

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

https://build.mozilla.org/tryserver-builds/2009-02-04_10:[e-mail]-pp4_ns/

Comment 21

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Awesome -- in that tryserver build, I've verified that bug 444448 and bug 476841 are both fixed. (as suggested in comment 15)

Comment 22

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

And btw, print preview zooming is also a lot faster with the patch.
Just try with HTML 5, ~680 pages.

Comment 23

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

(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?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 25

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

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.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, oops, I was confused. OK.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 28

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

Comment on attachment 360546 [details] [diff] [review]
no changes to scripting handling

Bah, I need to do something to nsTextFrame::PaintText

Comment 29

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

Attachment: a test case for text node change

Comment 30

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

Attachment: a test case for text node changes in SVG

Comment 31

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

Attachment: patch, approach 5

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.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 33

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

Yeah, that could be one option. I even tried it, or something similar, where
nsTextFrame with invalid data isn't painted.

Comment 34

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

http://java.sun.com/javase/6/docs/api/ is a good testcase for
print preview zoom performance.

Comment 35

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

(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 :(

Comment 36

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

Hmm, I need to check if not-deleting FirstLetterFrames would be ok

Comment 37

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

(In reply to comment #36)
> Hmm, I need to check if not-deleting FirstLetterFrames would be ok
Can't work the way I want.

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 39

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

Attachment: test case

Approach 5 has no problems with things like this.
Tested with DEBUG_smaug and print.debug.scripting_on = true

Comment 40

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

(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.

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 42

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

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.

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 44

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

(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.

Comment 45

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

(In reply to comment #44)
> and the make nsIFrame::mContent to point to the clone.
and then ...

Comment 46

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

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.

Comment 47

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

(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.

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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).

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 51

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

(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.

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Reflowing on a separate thread is less interesting because script so often needs to do synchronous reflow flushes.

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also, we're close to having interruptible reflow for non-flush reflows.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 55

[Boris Zbarsky [:bzbarsky]]

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.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 57

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

Attachment: test case. text, svg, input elements, :before/:after etc

Comment 58

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

Attachment: v7

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 363161 [details] [diff] [review]
v7

<grits teeth>

Comment 60

[Brandon Sterne (:bsterne)]

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.

Comment 61

[Boris Zbarsky [:bzbarsky]]

>+++ 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?

Comment 62

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

(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.

Comment 63

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

Attachment: v8

Comment 64

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 366100 [details] [diff] [review]
v8

Let's keep fingers crossed!

Comment 65

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

Asking blocking1.9.1.
See https://bugzilla.mozilla.org/show_bug.cgi?id=424377#c43

Comment 66

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

http://hg.mozilla.org/mozilla-central/rev/0e8572605937

Comment 67

[Daniel Holbert [:dholbert] (PTO until 2/24)]

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)

Comment 68

[Mike Beltzner [:beltzner, not reading bugmail]]

Same as bug 424377.

Comment 69

[Daniel Veditz [:dveditz]]

Comment on attachment 366100 [details] [diff] [review]
v8

This needs to land on 1.9.1 early to flush out regressions.

Comment 70

[Mike Beltzner [:beltzner, not reading bugmail]]

Olli: tests?

Comment 71

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

Ah, I thought I couldn't write automatic tests, but perhaps I can.
nsCanvasRenderingContext2D::DrawWindow uses the current preshell, not
primary shell.

Comment 72

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

Attachment: some tests for this bug and for bug 424377

Based on the tests for bug 396024.

Comment 73

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

Actually, how should we handle test cases for security bugs.
Perhaps better to push the tests after fixing this and bug 424377 on branch.

Comment 74

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 366100 [details] [diff] [review]
v8

a191=beltzner (push the tests and the fix all at the same time)

Comment 75

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

Pushed the test to trunk http://hg.mozilla.org/mozilla-central/rev/3414fd6ab52b

Comment 76

[Mike Beltzner [:beltzner, not reading bugmail]]

Re-adding the dependency I just removed. (Yay bugzilla and form value retention!)

Are we getting this on 191?

Comment 77

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

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.

Comment 78

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

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.

Comment 79

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

Attachment: followup for trunk

This moves #ifdef DEBUG

Comment 80

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

Comment on attachment 369762 [details] [diff] [review]
followup for trunk

http://hg.mozilla.org/mozilla-central/rev/ca1d87303a60

Comment 81

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

Attachment: for 1.9.1

Comment 82

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a049ed9eca86

Comment 83

[Daniel Veditz [:dveditz]]

Is this something we could get on the 1.9.0 branch without too much trouble?

Comment 84

[Daniel Veditz [:dveditz]]

Blocking assuming the answer looks like "yes"

Comment 85

[Daniel Veditz [:dveditz]]

We shouldn't take this on the branch until we're ready to take the whole group

Comment 86

[Daniel Veditz [:dveditz]]

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).