Closed Bug 813531 Opened 12 years ago Closed 12 years ago

An unknown error while printing SVG file

Categories

(Core :: SVG, defect)

17 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla20
Tracking Status
firefox17 --- wontfix
firefox18 + verified
firefox19 + verified
firefox20 + verified
firefox-esr17 - wontfix
b2g18 --- fixed

People

(Reporter: virgil.dicu, Assigned: jwatt)

References

Details

(Keywords: regression, verifyme)

Attachments

(7 files)

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 Build ID: 20121119074111 17.0 ESR Build ID: 20121119183901 17.0 RC build 2 Steps to reproduce: 1. Open http://upload.wikimedia.org/wikipedia/en/c/ce/SVG-logo.svg 2. Open File>Print or press Ctrl+P to print the logo 3. Press OK in the printer pop-up window Expected result: Logo should be printed. Actual result: An unknown error appears. Works for Google Chrome. Works for Firefox 16.0.2. This error is intermittent. Searching for regression range.
Regression range from mozregression Last good nightly: 2012-07-20 First bad nightly: 2012-07-21 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3a05d298599e&tochange=446b788ab99d
Perhaps a regression from bug 614723?
Keywords: regression
More likely bug Bug 614732
Blocks: 614732
Version: unspecified → 17 Branch
Assignee: nobody → jwatt
Why are we tracking this? I doubt printing bugs will bubble to the top of my priority queue any time soon.
(In reply to Jonathan Watt [:jwatt] from comment #4) > Why are we tracking this? I doubt printing bugs will bubble to the top of my > priority queue any time soon. Because it's a recent regression and prevents printing of SVGs.
Because this bug is seriously affecting the main functionality of our website I've been researching it a bit further. These are my conclusions: 1. Bigger svg filesize makes the problem occur more often, above 1mb allmost 100% 2. Printing files fast after another makes the problem occur more often 3. Using a slower/faster pc makes little difference 4. Firefox on Mac OSX 10.8.2 is not affected I've attached an SVG file which has an allmost 100% error occurrence. I hope this helps to get it up the queue a bit.
Attached image This file won't print (deleted) —
Trying to print this file will result in an error
Blocks: 818446
I'll dig into this after bug 786894 and bug 806432.
bug 818435 may be related.
(In reply to Jonathan Watt [:jwatt] (offline Thu-Sun) from comment #8) > I'll dig into this after bug 786894 and bug 806432. Agreed on prioritizing bug 786894 above this (new regression in 18), but I think we should fix this before bug 806432 given user impact.
+cc: dholbert for a look...
I tried to Print Preview the URL from comment 0 in my 64-bit Linux nightly & my debug build. I do see *some* of the content in the print-preview rendering -- the rightmost part of the "flower" logo is almost always visible, and sometimes the "SVG" text is, too. However: (a) If I scroll the "printed" page up and down, the SVG content moves on the page, almost as if it's trying to stay in my viewport... except it moves faster than I scroll, so it moves off the bottom of the screen as I scroll downward. (I have to my window shorter than the page, so that there's a scrollbar, in order to test this part.) (b) In my debug build, I get tons of instances of this warning as I scroll: WARNING: Outer object paint value used outside SVG glyph: file /scratch/work/builds/mozilla-central/mozilla-central.11-08-17.14-31/mozilla/layout/svg/nsSVGGlyphFrame.cpp, line 1060
Actually, I see that warning when simply viewing (and clicking around in) the SVG-logo.svg from comment 0 -- so that's not print-specific. Also: In my linux nightly, I can print mkopinga's attached SVG file, without any error-dialogs or anything, and the printout looks like the browser's rendering -- so that seems to be WORKSFORME. I can print SVG-logo.svg without any error-dialogs too, but the printout just shows the rightmost tip of the "flower" logo and nothing else, so it's still broken.
Thanks for taking a look at this, Daniel. I did a bit of my own digging and so far have tracked the issue down to the return of the NS_ERROR_FAILURE error code here: http://hg.mozilla.org/mozilla-central/annotate/352bd17710c8/layout/generic/nsSimplePageSequence.cpp#l587 This occurs under the following stack: nsSimplePageSequenceFrame::PrePrintNextPage nsPrintEngine::PrePrintPage nsPagePrintTimer::Notify nsTimerImpl::Fire I've yet to figure out why mCurrentPageFrame is null at that point.
It seems mCurrentPageFrame is null, not because someone resets it, but rather because the nsSimplePageSequenceFrame is destroyed and recreated without initializing it properly. (What's a bit confusing is that most of the time it gets recreated at the same address, making it look like it's the same nsSimplePageSequenceFrame.)
Okay, what seems to be happening here is that we hit this line (added in the final patch in bug 614732) that adds the nsChangeHint_ReconstructFrame hint to handle stacking context changes: http://hg.mozilla.org/mozilla-central/annotate/352bd17710c8/content/svg/content/src/nsSVGSVGElement.cpp#l935 under the stack: nsSVGSVGElement::ChildrenOnlyTransformChanged nsSVGOuterSVGFrame::NotifyViewportOrTransformChanged nsSVGOuterSVGFrame::Reflow nsContainerFrame::ReflowChild nsCanvasFrame::Reflow nsContainerFrame::ReflowChild nsPageContentFrame::Reflow nsContainerFrame::ReflowChild nsPageFrame::Reflow nsContainerFrame::ReflowChild nsSimplePageSequenceFrame::Reflow nsContainerFrame::ReflowChild ViewportFrame::Reflow PresShell::DoReflow PresShell::ProcessReflowCommands PresShell::FlushPendingNotifications PresShell::FlushPendingNotifications nsPrintEngine::ReflowPrintObject nsPrintEngine::ReflowDocList nsPrintEngine::InitPrintDocConstruction nsPrintEngine::DoCommonPrint nsPrintEngine::CommonPrint nsPrintEngine::Print nsDocumentViewer::Print We're hitting this because in nsSVGOuterSVGFrame::Reflow the viewport size is different for printing, resulting in the COORD_CONTEXT_CHANGED bit being added to changeBits, resulting in NotifyViewportOrTransformChanged being called.
I doesn't look like there's a simple way to try and reinitialize things (mCurrentPageFrame) when the nsSimplePageSequenceFrame is recreated.
roc, any ideas on how to best fix this? (Analysis from comment 14.)
We should not recreate the nsSimplePageSequenceFrame.
Depends on: 822378
Right now on the initial nsSVGOuterSVGFrame::Reflow() that method adds COORD_CONTEXT_CHANGED to changeBits, causing it to call NotifyViewportOrTransformChanged. If the outer-<svg> has a viewBox or synthesized viewBox, then NotifyViewportOrTransformChanged converts that to TRANSFORM_CHANGED, causing it to call nsSVGSVGElement::ChildrenOnlyTransformChanged passing the TRANSFORM_CHANGED. Since nsSVGSVGElement::mHasChildrenOnlyTransform is not at that point initialized (is set to false), ChildrenOnlyTransformChanged dispatches a nsChangeHint_ReconstructFrame for the nsSVGOuterSVGFrame, causing us to reconstruct the entire frame tree. This patch prevents us from calling NotifyViewportOrTransformChanged on the initial reflow, and has us just set nsSVGSVGElement::mHasChildrenOnlyTransform directly instead (the current code for setting that in nsSVGOuterSVGFrame::Reflow is bogus and doesn't actually do anything). Unfortunately, the undesirable frame tree reconstruction that was occurring under NotifyViewportOrTransformChanged has been hiding pre-existing bugs, so the patch results in various test failures by exposing them. One of those bugs is bug 822378 (patch landed). The remaining test failures I'm getting with this patch are: reftests/svg/as-image/img-foreignObject-1.html reftests/svg/as-image/img-foreignObject-embed-1.html reftests/svg/as-image/img-foreignObject-iframe-1a.html reftests/svg/as-image/img-foreignObject-iframe-1b.html Still working on sorting out what the issue is there.
Adding, needinfo for :Virgil & setting the QA contact as :juanb, to help with verfication here once this is ready.
Flags: needinfo?(virgil.dicu)
Keywords: qawanted, verifyme
QA Contact: jbecerra
(In reply to Jonathan Watt [:jwatt] from comment #23) > Still working on sorting out what the issue is there. So here's what's going on...there are two preexisting bugs that are masked by the reconstruct-the-frame-tree bug. Both manifest when the SVG is first reflowed for a zero sized viewport, and then later reflowed for a non-zero sized viewport. This is happening for the four foreignObject tests listed in comment 23 because SVGDocumentWrapper::OnStopRequest calls FlushLayout(), forcing reflow of the nsSVGOuterSVGFrame before its container document has had its initial reflow and set the dimensions of the frame for the embedding <img>. When the embedding frame for the <img> gets its first reflow, the nsSVGOuterSVGFrame is then reflowed at a non-zero size. The main pre-existing bugs is that when we reflow with a zero-sized viewport, we end up giving the direct children frames of the nsSVGOuterSVGFrame empty overflow areas. This is because FinishAndStoreOverflow takes account of the children-only transform on them, and that transform ends up being the identity matrix because that's what's returned by nsSVGSVGElement::GetViewBoxTransform() (under IsSVGTransformed) when viewportWidth/viewportHeight is zero. The bug is that we don't have any logic to make sure that we update these overflow areas when we reflow the nsSVGOuterSVGFrame with a valid viewport size. The other pre-existing bug is that in nsSVGOuterSVGFrame::NotifyViewportOrTransformChanged we discard the COORD_CONTEXT_CHANGED flag for synthetic viewBoxes. This is wrong because in the case of a synthetic viewBox the viewBox rect changes as the viewport changes (unlike an explicit viewBox's rect).
Attachment #693056 - Attachment description: WIP patch to stop reconstructing the frame tree on the initial reflow → patch to stop reconstructing the frame tree on the initial reflow
Attachment #693056 - Flags: review?(roc)
Comment on attachment 693197 [details] [diff] [review] patch to fix the two pre-existing bugs explained in comment 25 Review of attachment 693197 [details] [diff] [review]: ----------------------------------------------------------------- Got tests for these? ::: layout/svg/nsSVGOuterSVGFrame.cpp @@ +440,5 @@ > + anonChild->AddStateBits(NS_FRAME_IS_DIRTY); > + for (nsIFrame* child = anonChild->GetFirstPrincipalChild(); child; > + child = child->GetNextSibling()) { > + child->AddStateBits(NS_FRAME_IS_DIRTY); > + } You don't need this loop. NS_FRAME_IS_DIRTY forces all descendants to be reflowed too.
Attachment #693197 - Flags: review?(roc) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27) > Got tests for these? The four tests mentioned in comment 23. > You don't need this loop. NS_FRAME_IS_DIRTY forces all descendants to be > reflowed too. Actually ReflowSVG does not treat NS_FRAME_IS_DIRTY that way, so I do. Fixing up how SVG treats the dirty bits is on my list of things to do one day.
The current status after these two patches is that SVG files can now print without being blocked by the "An unknown error occurred while printing" error message. That said, not all SVG files are quite printing correctly. The attachment "This file won't print" now prints fine, but the SVG-logo.svg example from comment 0 seems to be printing with the wrong offset. I'm not sure yet at which point between the landing of bug 614732 and now that offset was broken since nightly builds in that time range can't print, of course. It's possible that bug 614732 broke the offset too. Anyway, since the original blocking error message is now fixed I'm not sure whether I should leave this open for the offset bug, or open a different bug for that.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 693056 [details] [diff] [review] patch to stop reconstructing the frame tree on the initial reflow [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 614732 User impact if declined: SVG with a viewBox (fairly common) won't print Testing completed (on m-c, etc.): landed on m-c Risk to taking this patch (and alternatives if risky): low enough risk for beta 5 I think String or UUID changes made by this patch: none
Attachment #693056 - Flags: approval-mozilla-beta?
Attachment #693056 - Flags: approval-mozilla-aurora?
Attachment #693197 - Flags: approval-mozilla-beta?
Attachment #693197 - Flags: approval-mozilla-aurora?
Comment on attachment 693056 [details] [diff] [review] patch to stop reconstructing the frame tree on the initial reflow Approving on branches.Confirmed with :jwatt about the risk being low.In addition, we will request QA to do a lot of SVG testing for beta 5
Attachment #693056 - Flags: approval-mozilla-beta?
Attachment #693056 - Flags: approval-mozilla-beta+
Attachment #693056 - Flags: approval-mozilla-aurora?
Attachment #693056 - Flags: approval-mozilla-aurora+
Attachment #693197 - Flags: approval-mozilla-beta?
Attachment #693197 - Flags: approval-mozilla-beta+
Attachment #693197 - Flags: approval-mozilla-aurora?
Attachment #693197 - Flags: approval-mozilla-aurora+
Juan,Virgil : can you guys please have explicit test cases/testing around SVG for FF18 beta 5 , around this bug. Thanks !
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/20100101 Firefox/18.0 Marking as verified in beta 5, the initial issue is no longer reproducible. Print preview/landascape/portrait - all print correctly. Verified with Ubuntu 12.04, Windows 7, Mac OS 10.7. Also ran related SVG test cases (listed in the Firefox 18b5 Test Plan) - all looks good. https://wiki.mozilla.org/Releases/Firefox_18/Test_Plan#SVG_Testing
Flags: needinfo?(virgil.dicu)
Keywords: qawanted
Blocks: 818435
This is reproducing on 17.0.2 ESR. Will it be ported on 17ESR?
Verified the fix in Firefox 19.0 Beta 2 - no longer reproducible on Windows 7, Mac OS X 10.7.5 and Ubuntu 12.04. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0 Build ID: 20130116072953
Verified Fixed on FF 20.b2 Win 7 x64, Ubuntu 12.04 x86 and Mac OS 10.8
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: