Closed
Bug 717178
Opened 12 years ago
Closed 11 years ago
Printing stops after 73%, and browser hangs up
Categories
(Core :: Graphics, defect)
Tracking
()
VERIFIED
FIXED
mozilla21
People
(Reporter: franz.ehrenhuber, Assigned: roc)
References
(Blocks 1 open bug)
Details
(Keywords: hang, regression)
Attachments
(4 files, 4 obsolete files)
76.18 KB,
application/octet-stream
|
Details | |
1.10 KB,
patch
|
jfkthame
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
11.83 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
11.91 KB,
patch
|
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1 Build ID: 20111220165912 Steps to reproduce: Printing a local html page. Actual results: Printing stops after 73% and firefox is frozen Expected results: Printing continues to 100% and document is sent to the printer
Reporter | ||
Comment 1•12 years ago
|
||
I have added the html file used to do printing and a screenshot about last status when firefox stops working
Comment 2•12 years ago
|
||
Regression window(m-c) Works: http://hg.mozilla.org/mozilla-central/rev/5faaa652594f Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110915 Firefox/9.0a1 ID:20110915013748 Fails: http://hg.mozilla.org/mozilla-central/rev/c6a405d01a1d Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110915 Firefox/9.0a1 ID:20110915064148 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5faaa652594f&tochange=c6a405d01a1d Regression window(m-i) Works: http://hg.mozilla.org/integration/mozilla-inbound/rev/7e132eee5650 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110914 Firefox/9.0a1 ID:20110914103830 Fails: http://hg.mozilla.org/integration/mozilla-inbound/rev/c614dabf94e2 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110914 Firefox/9.0a1 ID:20110914115129 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7e132eee5650&tochange=c614dabf94e2 Suspected: fec42fae34a7 Adrian Johnson — Bug 454532. cairo: Print as Unicode. r=jrmuizel
Blocks: 454532
Status: UNCONFIRMED → NEW
Component: Untriaged → Graphics
Ever confirmed: true
Keywords: hang,
regression
Product: Firefox → Core
QA Contact: untriaged → thebes
Hardware: x86_64 → x86
Comment 3•12 years ago
|
||
It works properly if I disable HWA
Updated•12 years ago
|
Severity: normal → critical
Updated•12 years ago
|
Summary: Printing stops after 73% → Printing stops after 73%, and browser hangs up
Reporter | ||
Comment 4•12 years ago
|
||
Just verified that. If I disable HWA without closing the browser and printing again, FF crashes. After restart, printing works fine. But there seems to be a seriuos problem, with HWA and printing. I think you can not tell the user, 'If you want to print then disable HWA' This should work anyway!
Comment 5•12 years ago
|
||
Adrian, can you take a look at this?
Comment 7•12 years ago
|
||
Reproduced with: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1 Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:12.0a1) Gecko/20120111 Firefox/12.0a1
Comment 8•12 years ago
|
||
This only occurs with the DirectWrite fontlist code. Bug 454532 was a change to the GDI fontlist code, so it's not possible that that bug caused this. I think this is a combination of an error in our printing code that is somehow causing the hang detection code to put the event handling code in a big spin loop. Running this in the debugger, that appears to be whats happening but event handling code is a bit of a mystery to me so I can't assert this for sure.
Comment 9•12 years ago
|
||
Loop in printing code: The code in nsPrintEngine::PrintPage calls nsSimplePageSequenceFrame::DoPageEnd: http://mxr.mozilla.org/mozilla-central/source/layout/printing/nsPrintEngine.cpp#2521 The code in this method executes: rv = PresContext()->DeviceContext()->EndPage(); This fails but the error returned is ignored by the calling method, PrintPage. An infinite loop results.
Comment 10•12 years ago
|
||
Argh, missed one of the changesets on that bug. Probable cause (from bug 454532): https://hg.mozilla.org/mozilla-central/rev/fec42fae34a7
Blocks: 454532
Comment 11•12 years ago
|
||
Confirmed that with the changeset just before the one noted in comment 10, the testcase attached here prints out completely. After the patch, the UI stalls at 67% and only 11 out of 18 pages print (with default page size set to A4). The curious thing is that before the change was landed, the progress dialog appears only briefly. After the change it appears for several seconds before the progress hits 67% and stalls.
Comment 12•12 years ago
|
||
I can reproduce this by setting gfx.direct2d.force-enabled to true (my laptop has Intel 965 graphics). I don't know how to get the debugger to work with firefox so I can't see exactly where it is failing. However the most useful clue is the fact it only occurs when using DirectWrite fonts. Looking at how the win32 printing surface handles dwrite fonts I see that _cairo_dwrite_scaled_font_create_win32_scaled_font() is being called on every call to cairo_show_glyphs() to create a new win32 font face and scaled font. This font face and scaled font are not destroyed at the end of the show_glyphs call because a reference is held by the win32 printing surface until the page is finished. It is possible that after creating too many HFONT objects GDI calls start failing. There are some patches in cairo git master to use a hash table of win32 font faces to avoid creating a new font face if there is already an existing font face with the same LOGFONT. After applying the following patches from cairo git master: eb29a25d 6e3e3291 101fab7c The test case now works for me.
Comment 13•12 years ago
|
||
Jeff, is there a cairo update plan that would include the patches Adrian mentions?
Comment 16•12 years ago
|
||
I had the same problem, solved by disabled HWA. I did a lot of testing before I found the solution and found the exact document size that would cause this failure to occur: 106187 OK, 106188 ERROR. This was on a different document that unfortunately I cannot share. When printing to file, the resulting size when the error occurs is 716 kb.
Comment 17•12 years ago
|
||
I have the same issue . My page will be 33 pages after print but it is waiting on page 12 then I delete on going print queue it will print that 12 pages but firefox freeze and I have to close and reopen it.
Comment 18•12 years ago
|
||
Jeff, do you think you could find someone to look at this?
Comment 19•12 years ago
|
||
(In reply to John Daggett (:jtd) from comment #9) > Loop in printing code: > > The code in nsPrintEngine::PrintPage calls > nsSimplePageSequenceFrame::DoPageEnd: > > http://mxr.mozilla.org/mozilla-central/source/layout/printing/nsPrintEngine. > cpp#2521 > > The code in this method executes: > > rv = PresContext()->DeviceContext()->EndPage(); > > This fails but the error returned is ignored by the calling method, > PrintPage. An infinite loop results. Need to fix this one sure, but also the printing problem.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → roc
Assignee | ||
Comment 21•12 years ago
|
||
Attachment #672685 -
Flags: review?(jfkthame)
Assignee | ||
Comment 22•12 years ago
|
||
Attachment #672686 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 23•12 years ago
|
||
Thanks Adrian! These patches fix the testcase for me too.
Comment 24•12 years ago
|
||
Comment on attachment 672685 [details] [diff] [review] Part 1: Don't crash when passing a nil scaled-font to _name_tables_match. r=jfkthame Review of attachment 672685 [details] [diff] [review]: ----------------------------------------------------------------- I guess this happens when windows fails to create the new GDI font resource? I don't think it should be possible for the "original" dwrite font to have a nil backend here, as we've already been accessing it before getting to this point. Still, the simplest and safest fix here is just to check both fonts, rather than making (fragile) assumptions about which of them is "known" to be good.
Attachment #672685 -
Flags: review?(jfkthame) → review+
Updated•12 years ago
|
Attachment #672686 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 25•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/40e9ab16d3b2 https://hg.mozilla.org/integration/mozilla-inbound/rev/20279718e3c7
Assignee | ||
Comment 26•12 years ago
|
||
Comment on attachment 672685 [details] [diff] [review] Part 1: Don't crash when passing a nil scaled-font to _name_tables_match. r=jfkthame [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 802476 User impact if declined: crashes when printing Testing completed (on m-c, etc.): none yet Risk to taking this patch (and alternatives if risky): this patch is incredibly low risk, just a couple of null checks. No real alternatives. String or UUID changes made by this patch: none.
Attachment #672685 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 27•12 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26) > Bug caused by (feature/regressing bug #): bug 802476 This is wrong, it's actually bug 468568.
Assignee | ||
Comment 28•12 years ago
|
||
Part 2 caused test failures on Windows XP so I backed it out. https://tbpl.mozilla.org/php/getParsedLog.php?id=16366499&tree=Mozilla-Inbound&full=1 It looks like GDI text rendering just stopped working.
Whiteboard: [leave open]
Comment 29•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/40e9ab16d3b2
Flags: in-testsuite?
Assignee | ||
Comment 30•12 years ago
|
||
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bidi/263359-1b.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/40596-1h.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/180085-1.html | image comparison (==), max difference: 255, number of differing pixels: 3042 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/180085-2.html | image comparison (==), max difference: 255, number of differing pixels: 2366 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/296361-1.html | image comparison (==), max difference: 255, number of differing pixels: 30 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/315920-1c.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/315920-1d.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/315920-3e.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/315920-3f.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/338251-pre.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/341043-1b.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/350506-1.html | image comparison (==), max difference: 255, number of differing pixels: 18 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/411059-1.html | image comparison (==), max difference: 255, number of differing pixels: 19608 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/455826-1.html | image comparison (==), max difference: 255, number of differing pixels: 194 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/475986-1-ref.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/475986-1-ref.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/bugs/475986-2-ref.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/css-placeholder/input/placeholder-add.html | image comparison (==), max difference: 128, number of differing pixels: 111 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/css-placeholder/input/placeholder-value-unset.html | image comparison (==), max difference: 128, number of differing pixels: 111 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/css-placeholder/input/placeholder-value-reset.html | image comparison (==), max difference: 128, number of differing pixels: 111 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/css-placeholder/input/placeholder-type-change-1.html | image comparison (==), max difference: 128, number of differing pixels: 111 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/flexbox/flexbox-float-1c.xhtml | image comparison (==), max difference: 238, number of differing pixels: 47 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/flexbox/flexbox-float-1d.xhtml | image comparison (==), max difference: 238, number of differing pixels: 47 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/font-face/local-styled-1.html | assertion count 2 is more than expected 0 assertions REFTEST TEST-UNEXPECTED-FAIL | view-source:file:///C:/talos-slave/test/build/reftest/tests/parser/htmlparser/tests/reftest/bug482921-1.html | image comparison (==), max difference: 255, number of differing pixels: 18 REFTEST TEST-UNEXPECTED-FAIL | view-source:file:///C:/talos-slave/test/build/reftest/tests/parser/htmlparser/tests/reftest/bug482921-2.xhtml | image comparison (==), max difference: 255, number of differing pixels: 18 REFTEST TEST-UNEXPECTED-FAIL | view-source:file:///C:/talos-slave/test/build/reftest/tests/parser/htmlparser/tests/reftest/bug700260-1.html | image comparison (==), max difference: 255, number of differing pixels: 432 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/tab-size/tab-size-8.html | image comparison (==), max difference: 255, number of differing pixels: 126 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/tab-size/tab-size-4.html | image comparison (==), max difference: 255, number of differing pixels: 54 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/tab-size/tab-size-4-span.html | image comparison (==), max difference: 255, number of differing pixels: 54 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/tab-size/tab-size-4-spanoffset.html | image comparison (==), max difference: 255, number of differing pixels: 216 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/tab-size/tab-size-4-multiple.html | image comparison (==), max difference: 255, number of differing pixels: 72 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/layout/reftests/tab-size/tab-size-1.html | image comparison (==), max difference: 255, number of differing pixels: 18 REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-attr.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-nofocus.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-disabled.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-attr-inherit.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-attr-dynamic.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-attr-dynamic-inherit.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-property-dynamic.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-property-dynamic-inherit.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-attr-dynamic-override.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-attr-dynamic-override-inherit.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-property-dynamic-override.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-textarea-property-dynamic-override-inherit.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-hyphen-invalid.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-hyphen-multiple-invalid.html | image comparison (!=) REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/editor/reftests/spellcheck-superscript-2.html | image comparison (!=)
Assignee | ||
Comment 31•12 years ago
|
||
Interestingly, disabling D2D on Windows 7 doesn't seem to exhibit the bug.
Comment 32•12 years ago
|
||
Waiting on the test failures to get resolved before approving on Aurora .
Assignee | ||
Comment 33•12 years ago
|
||
I can't reproduce in my Windows XP VM either.
Assignee | ||
Updated•12 years ago
|
Attachment #672685 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 34•12 years ago
|
||
Comment on attachment 672685 [details] [diff] [review] Part 1: Don't crash when passing a nil scaled-font to _name_tables_match. r=jfkthame [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 802476 User impact if declined: crashes when printing Testing completed (on m-c, etc.): patch on central for a day Risk to taking this patch (and alternatives if risky): this patch is incredibly low risk, just a couple of null checks. No real alternatives. The other patch in this bug caused test failures, but this patch did not and is on central now and should fix the crash regressions. String or UUID changes made by this patch: none.
Attachment #672685 -
Flags: approval-mozilla-aurora?
Comment 35•12 years ago
|
||
Comment on attachment 672685 [details] [diff] [review] Part 1: Don't crash when passing a nil scaled-font to _name_tables_match. r=jfkthame Approving this patch for Aurora as it fixes crashes when printing
Attachment #672685 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 36•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/3469c641a728
Comment 39•12 years ago
|
||
Looks to me like this landed in 19, should it be marked fixed for that?
Assignee | ||
Comment 40•11 years ago
|
||
If you like
Comment 41•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28) > Part 2 caused test failures on Windows XP so I backed it out. > > https://tbpl.mozilla.org/php/getParsedLog.php?id=16366499&tree=Mozilla- > Inbound&full=1 > > It looks like GDI text rendering just stopped working. I guess that's why it's still open, is that still to be addressed?
Comment 42•11 years ago
|
||
I can still reproduce the problem of comment #0 in Firefox18beta,19.0a2 and 20.0a1. http://hg.mozilla.org/releases/mozilla-beta/rev/6eb5edeb9f52 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 ID:20121128060531 http://hg.mozilla.org/releases/mozilla-aurora/rev/8e070b999a77 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20121202 Firefox/19.0 ID:20121202042013 http://hg.mozilla.org/mozilla-central/rev/0352a32fde64 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121202 Firefox/20.0 ID:20121202030723
Assignee | ||
Comment 43•11 years ago
|
||
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #41) > I guess that's why it's still open, is that still to be addressed? Yeah, that's right.
Assignee | ||
Comment 45•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8e4a7760e0a1
Assignee | ||
Comment 46•11 years ago
|
||
I don't understand how cairo_font_face_t lifetime management works with this cairo patch. We call cairo_win32_font_face_create_for_logfontw_hfont. It creates a hashtable entry. What ever removes that entry from the hashtable? As far as I can tell, nothing ever does, even if the cairo_win32_font_face_t dies.
Assignee | ||
Comment 47•11 years ago
|
||
That's easy to fix by removing the cairo_win32_font_face_t from the hashtable when it's destroyed. But we seem to have another problem, in gfxGDIFont's destructor. We unreference the cairo_win32_font_face_t and then call DeleteObject on mFont. But we don't actually know whether the cairo_win32_font_face_t was deleted; it may still be alive and in use somewhere (perhaps due to being returned from the hashtable by a different call by cairo_win32_font_face_create_for_logfontw_hfont). If it's still in use, then the call to DeleteObject on gfxGDIFont::mFont means the cairo_win32_font_face_t's mFont is bogus and later attempts to use it will fail.
Assignee | ||
Comment 48•11 years ago
|
||
So basically right now there is no guarantee that the HFONT passed to cairo_win32_font_face_create_for_logfontw_hfont will remain alive as long as it is needed by the cairo_font_face_t we return.
Assignee | ||
Comment 49•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=5463097921d2
Assignee | ||
Comment 50•11 years ago
|
||
Build failure due to some symbol export thing. Re-pushed: https://tbpl.mozilla.org/?tree=Try&rev=726cf95974f7
Comment 51•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #47) > But we seem to have another problem, in gfxGDIFont's destructor. We > unreference the cairo_win32_font_face_t and then call DeleteObject on mFont. > But we don't actually know whether the cairo_win32_font_face_t was deleted; You call cairo_font_face_set_user_data() after creating the font_face to set the function to be called when the font_face is destroyed.
Assignee | ||
Comment 52•11 years ago
|
||
You mean we could, but we currently don't :-). Yes, we could do that. But it seems silly to make that a requirement for using cairo_win32_font_face_create_for_logfontw_hfont safely.
Assignee | ||
Comment 53•11 years ago
|
||
Updated to remove the cairo_font_face_t from the hashtable when it is destroyed.
Attachment #672686 -
Attachment is obsolete: true
Attachment #689625 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 54•11 years ago
|
||
This is an API change I guess, but the previous API was nigh impossible to use correctly.
Attachment #689626 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 55•11 years ago
|
||
Reftests are green.
Assignee | ||
Comment 56•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #54) > This is an API change I guess, but the previous API was nigh impossible to > use correctly. If this API change is deemed unacceptable, I'll rework the patch to use the user_data approach.
Updated•11 years ago
|
Attachment #689625 -
Flags: review?(jmuizelaar) → review+
Comment 57•11 years ago
|
||
Comment on attachment 689626 [details] [diff] [review] Part 3: Pass ownership of HFONT to the cairo_scaled_font_t Review of attachment 689626 [details] [diff] [review]: ----------------------------------------------------------------- Sorry this took so long.
Attachment #689626 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 58•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cd447a60faa https://hg.mozilla.org/integration/mozilla-inbound/rev/b1453ec5b550 https://hg.mozilla.org/integration/mozilla-inbound/rev/4bab6b6a32f3
Whiteboard: [leave open]
Assignee | ||
Comment 59•11 years ago
|
||
Attachment #696854 -
Flags: review?(jmuizelaar)
Comment 60•11 years ago
|
||
Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/1322a7cadd56 for: https://tbpl.mozilla.org/php/getParsedLog.php?id=18382182&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=18383599&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=18382584&tree=Mozilla-Inbound
Updated•11 years ago
|
Attachment #696854 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 61•11 years ago
|
||
The mochitest failure shows a real bug. The problem is that with these patches, gfxGDIFont's mFont is no longer reliable since if we match a cached cairo_font_face_t, we'll delete the passed-in HFONT immediately. But the dead mFont is still stored in gfxGDIFont, and used in various places afterwards. We could try to fix that, but I think it might be better to reduce code changes here and simply not cache anything where we had a passed-in HFONT. The cache setup simply doesn't work well in that case.
Assignee | ||
Comment 62•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e3204f273eed
Assignee | ||
Comment 63•11 years ago
|
||
That was screwed up. https://tbpl.mozilla.org/?tree=Try&rev=130bd2bc1ccc
Assignee | ||
Comment 64•11 years ago
|
||
This is green on try. I still think the cairo APIs that take an HFONT are broken; it's just bad to force the caller to ensure that all references to the refcounted cairo_font_face_t are dropped (including references via scaled fonts, caches, etc) before the caller destroys the HFONT. However, fixing that API issue seems like more trouble than it's worth.
Attachment #696977 -
Flags: review?(jmuizelaar)
Comment 65•11 years ago
|
||
Comment on attachment 696977 [details] [diff] [review] alternative to parts 3 and 4 Review of attachment 696977 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/cairo/cairo/src/cairo-win32-font.c @@ +1,1 @@ > +/* -*- Mode: c; tab-width: 8; c-basic-offset: 4; indent-tabs-mode: t; -*- */ ?
Attachment #696977 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 66•11 years ago
|
||
Attachment #698801 -
Flags: checkin?
Assignee | ||
Updated•11 years ago
|
Attachment #689626 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #696854 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #696977 -
Attachment is obsolete: true
Assignee | ||
Comment 67•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef465cf5fc12 https://hg.mozilla.org/integration/mozilla-inbound/rev/1435e8a04081
Comment 68•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ef465cf5fc12 https://hg.mozilla.org/mozilla-central/rev/1435e8a04081
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Assignee | ||
Comment 69•11 years ago
|
||
Printing not working is pretty bad.
status-firefox19:
--- → affected
tracking-firefox19:
--- → ?
Assignee | ||
Comment 70•11 years ago
|
||
Actually, don't bother tracking for 19. This regressed in Firefox 9 so it's not worth putting a fix into beta.
tracking-firefox19:
? → ---
Assignee | ||
Comment 71•11 years ago
|
||
Comment on attachment 698801 [details] [diff] [review] alternative to parts 3 and 4, v2, for checkin [Approval Request Comment] Bug caused by (feature/regressing bug #): 454532 User impact if declined: Printing some pages fails; printing other pages might consume lots of memory Testing completed (on m-c, etc.): just landed Risk to taking this patch (and alternatives if risky): a bit risky, so requesting only Aurora approval String or UUID changes made by this patch: none
Attachment #698801 -
Flags: checkin? → approval-mozilla-aurora?
Comment 72•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #71) > Comment on attachment 698801 [details] [diff] [review] > alternative to parts 3 and 4, v2, for checkin > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): 454532 > User impact if declined: Printing some pages fails; printing other pages > might consume lots of memory > Testing completed (on m-c, etc.): just landed > Risk to taking this patch (and alternatives if risky): a bit risky, so > requesting only Aurora approval > String or UUID changes made by this patch: none roc, considering the risk profile here and based on the patch do we know the severity of regressions this may cause? This has been there since Fx9, do you think letting this ride the trains would be of any help here in terms of getting more testing ?
Assignee | ||
Comment 73•11 years ago
|
||
(In reply to bhavana bajaj [:bajaj] from comment #72) > roc, considering the risk profile here and based on the patch do we know the > severity of regressions this may cause? The severity is that some pages (probably any sufficiently long and complex page) can't be printed in Firefox on Windows 7. You can see there are six duplicates filed. > This has been there since Fx9, do > you think letting this ride the trains would be of any help here in terms of > getting more testing ? It would, of course. I think the latest patches are pretty low-risk --- we bypass the new caching code for almost all our font usage.
Comment 74•11 years ago
|
||
Comment on attachment 698801 [details] [diff] [review] alternative to parts 3 and 4, v2, for checkin Looks like this hasn't landed yet. Should this be reopened?
Comment 75•11 years ago
|
||
AFAICT it's landed on inbound/m-c (1435e8a04081, the second changeset in comments 67-68), but awaiting approval for aurora.
Comment 76•11 years ago
|
||
Comment on attachment 698801 [details] [diff] [review] alternative to parts 3 and 4, v2, for checkin Approving, the alternative low risk patches for aurora. We have also requested QA testing/verification on this issue .
Attachment #698801 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•11 years ago
|
status-firefox20:
--- → affected
tracking-firefox20:
--- → +
Comment 77•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/0f5e486ffd77 https://hg.mozilla.org/releases/mozilla-aurora/rev/64b4d4cb1f06
status-firefox21:
--- → fixed
Comment 78•11 years ago
|
||
Is it too late to get this patched in 19? My users have encountered this problem, loaded up Aurora on my pc, and it fixed it, but I'd rather not have Aurora on the user's computers.
Assignee | ||
Comment 79•11 years ago
|
||
I think it is, sorry. This bug has been around for over a year so the danger of ramming it into FF19 doesn't seem justified by getting it fixed six weeks earlier.
Comment 80•11 years ago
|
||
Reproduced on FF 19. Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0b1
Comment 81•11 years ago
|
||
Verified fixed Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0b1
Comment 82•11 years ago
|
||
I can reproduce this issue with Firefox 17.0.9 ESR on Win 7 64bit, stopping at 96%. Does anyone have any thoughts/suggestions? Thanks!
Comment 83•11 years ago
|
||
That's expected, AFAICS. This was fixed for Firefox 20, and was never nominated for backporting to ESR-17.
Comment 84•11 years ago
|
||
It would be nice to have this on ESR too. Meanwhile, large files can't be printed.
Comment 85•11 years ago
|
||
If it's sufficiently important, you could nominate it for esr tracking/uplift -- though assuming esr24 will be appearing "real soon now" (next week?), I think it's questionable whether it would be backported to esr17 at this point. Perhaps more important would be to verify that the problem does *not* occur on esr24, as that's the version we're about to ship.
Comment 86•11 years ago
|
||
Yes, ESR 24 is shipping next week. I don't think anyone will approve backporting this to ESR 17 at this point. People who need this should update to ESR 24 as soon as they can.
You need to log in
before you can comment on or make changes to this bug.
Description
•