Blob images don't work on Windows

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jrmuizel, Assigned: lsalzman)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 attachments, 4 obsolete attachments)

The dwrite fonts depend on some stuff from gfxPlatform which we intentionally don't have in the GPU process.
Lee, any thoughts or feelings about what we should do?
Flags: needinfo?(lsalzman)
Posted patch Make things hobble along (obsolete) — Splinter Review
Assignee: nobody → jmuizelaar
(In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> Lee, any thoughts or feelings about what we should do?

Would possibly make more sense to pass in the gamma and contrast as parameters to SkCreateTypefaceFromDWriteFont, and then do the resolution of those values higher up, probably in thebes, which would mean plumbing to pass them along in ScaledFontDWrite as well.
Flags: needinfo?(lsalzman)
(In reply to Lee Salzman [:lsalzman] from comment #3)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #1)
> > Lee, any thoughts or feelings about what we should do?
> 
> Would possibly make more sense to pass in the gamma and contrast as
> parameters to SkCreateTypefaceFromDWriteFont, and then do the resolution of
> those values higher up, probably in thebes, which would mean plumbing to
> pass them along in ScaledFontDWrite as well.

I'll take a look into doing this during the all hands. Should be something that's up my alley.
Discussed with Jeff. I'll work on fixing this.
Assignee: jmuizelaar → lsalzman
Priority: -- → P3
Whiteboard: [gfx-noted]
This patch ranges a bit far through the code, but it is mostly all in the service of one theme: sanitizing the DWrite plumbing.

Firstly, we had dwrite.dll loading happening in multiple places, so Factory::EnsureDWriteFactory was added to centralize this. Since it needs to be called from multiple threads, it required locking to be safe.

Secondly, we need to ensure that Skia isn't calling back into gfxWindowsPlatform to get the dwrite parameters (gamma and contrast). Since these are associated with the font, I added plumbing in ScaledFontDWrite to just store these and pass these in during Skia typeface creation. I used the InstanceData mechanism I had added for ScaledFontFonconfig for this so that we can serialize the gamma/contrast and send it through the recording stream without having to query this from the DWrite on the other side.

This way, Skia has everything it needs and does not need to call back to Gecko to get it. We also no longer need GlyphRenderingOptionsDWrite for this purpose, which made little sense anyway given that these parameters were strictly associated with the font, and not the text run being rendered.

Thirdly, CreateNativeFontResource was cleaned up so that it now always has a specific defined type of font resource to create that gets recorded. This was necessary since on the playback side we can't always guarantee we'll reproduce the correct decision-making that selected whether to use DWrite or GDI, so better to just write in the stream if we started with DWrite or GDI so the correct decision is always made.
Attachment #8882599 - Flags: review?(jmuizelaar)
Status: NEW → ASSIGNED
After discussion with Mason, who added a check for D2D support before using DWrite to gfxWindowsPlatform, it turns out the MSDN documentation, in its own hard to understand way, was not actually meaning that we would need D2D support for our current usage of DWrite. So we can just remove this check to simplify our code.
Attachment #8880969 - Attachment is obsolete: true
Attachment #8882599 - Attachment is obsolete: true
Attachment #8882599 - Flags: review?(jmuizelaar)
Attachment #8882620 - Flags: review?(jmuizelaar)
V2 that now just assumes if a DWrite factory can be created, then we should use DWrite, without requiring D2D support.
Attachment #8882621 - Flags: review?(jmuizelaar)
V3, add a comment describing why we need to store both the IDWriteRenderingParams and gamma/contrast values for use in DrawTargetD2D1 and DrawTargetSkia.
Attachment #8882621 - Attachment is obsolete: true
Attachment #8882621 - Flags: review?(jmuizelaar)
Attachment #8882624 - Flags: review?(jmuizelaar)
Attachment #8882620 - Flags: review?(jmuizelaar) → review+
Attachment #8882624 - Flags: review?(jmuizelaar) → review+
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a93c71a7ef8b
assume DWrite is available on Windows 7 even without the platform update. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d09fb3a5cedf
fix plumbing of DWrite parameters for Skia fonts to not depend on gfxPlatform. r=jrmuizel
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/af8c0e8089fc
fix ScaledFontDWrite::InstanceData constructor to be explicit. r=me
erm s/the other bug/ the followups :)
Fixed some build failures.
Attachment #8882624 - Attachment is obsolete: true
Flags: needinfo?(lsalzman)
Attachment #8884339 - Flags: review+
Our Win7 VM test machines were not using DWrite previously, so enabling DWrite with it (on any Win7 machine) caused some diverging test results. This fixes some fuzz and unexpected passes.
Attachment #8884340 - Flags: review?(jmuizelaar)
Attachment #8884340 - Flags: review?(jmuizelaar) → review+
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be7d6aa4aef3
assume DWrite is available on Windows 7 even without the platform update. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/24421d72ba08
fix plumbing of DWrite parameters for Skia fonts to not depend on gfxPlatform. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/387e4a353e65
fix tests to assume that Windows 7 always uses DWrite. r=jrmuizel
Backed out for failing browser_bug970746.js on Windows 7 VM:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b64554d7181e07f1d8dd7718c211acbf68d2a761
https://hg.mozilla.org/integration/mozilla-inbound/rev/39cb5de383f45c85f3938ebe6cd12e6631bf4773
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5912642f421bdd6061be45621fc0d94e675be6f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b64554d7181e07f1d8dd7718c211acbf68d2a761&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=112645434&repo=mozilla-inbound

18:42:16     INFO -  394 INFO TEST-PASS | browser/base/content/test/general/browser_bug970746.js | Menu item text 'Search Google for “I'm some text, …”' contains the correct search terms 'I'm some text, …' -
18:42:16     INFO -  395 INFO TEST-PASS | browser/base/content/test/general/browser_bug970746.js | search context menu item is shown for  '#mixedContent' and selected is 'false' -
18:42:16     INFO -  396 INFO TEST-PASS | browser/base/content/test/general/browser_bug970746.js | search context menu item is shown for  '#partialLink' and selected is 'true' -
18:42:16     INFO -  397 INFO TEST-PASS | browser/base/content/test/general/browser_bug970746.js | Menu item text 'Search Google for “link selection”' contains the correct search terms 'link selection' -
18:42:16     INFO -  Buffered messages finished
18:42:16    ERROR -  398 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug970746.js | search context menu item is shown for  '#partialLink' and selected is 'false' - Got true, expected false
18:42:16     INFO -  Stack trace:
18:42:16     INFO -  chrome://mochikit/content/browser-test.js:test_is:967
18:42:16     INFO -  chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug970746.js:null:106
Flags: needinfo?(lsalzman)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #19)
> Backed out for failing browser_bug970746.js on Windows 7 VM:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> b64554d7181e07f1d8dd7718c211acbf68d2a761
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 39cb5de383f45c85f3938ebe6cd12e6631bf4773
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> e5912642f421bdd6061be45621fc0d94e675be6f
> 
> Push with failures:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=b64554d7181e07f1d8dd7718c211acbf68d2a761&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=retry&filter-
> resultStatus=usercancel&filter-resultStatus=runnable
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=112645434&repo=mozilla-
> inbound
> 
> 18:42:16     INFO -  394 INFO TEST-PASS |
> browser/base/content/test/general/browser_bug970746.js | Menu item text
> 'Search Google for “I'm some text, …”' contains the correct search terms
> 'I'm some text, …' -
> 18:42:16     INFO -  395 INFO TEST-PASS |
> browser/base/content/test/general/browser_bug970746.js | search context menu
> item is shown for  '#mixedContent' and selected is 'false' -
> 18:42:16     INFO -  396 INFO TEST-PASS |
> browser/base/content/test/general/browser_bug970746.js | search context menu
> item is shown for  '#partialLink' and selected is 'true' -
> 18:42:16     INFO -  397 INFO TEST-PASS |
> browser/base/content/test/general/browser_bug970746.js | Menu item text
> 'Search Google for “link selection”' contains the correct search terms 'link
> selection' -
> 18:42:16     INFO -  Buffered messages finished
> 18:42:16    ERROR -  398 INFO TEST-UNEXPECTED-FAIL |
> browser/base/content/test/general/browser_bug970746.js | search context menu
> item is shown for  '#partialLink' and selected is 'false' - Got true,
> expected false
> 18:42:16     INFO -  Stack trace:
> 18:42:16     INFO -  chrome://mochikit/content/browser-test.js:test_is:967
> 18:42:16     INFO - 
> chrome://mochitests/content/browser/browser/base/content/test/general/
> browser_bug970746.js:null:106

Jared, it would appear that the test introduced in bug 970746 is failing here for a reason that I can't really discern, but only on the Windows 7 VM test machine. What is special about this category of test machine is it does not support Direct2D, and as of this bug, we would be enabling DirectWrite on it. Why this would cause the search-select item in the context menu to not show up is beyond me, as on a Windows 7 machine with Direct2D enabled, I am not able to reproduce this scenario locally, and there is nothing superficially wrong with the test.

I can't see any obvious reason why changing from GDI fonts to DirectWrite fonts would cause an unrelated context menu item to not show up, and only in that particular scenario without Direct2D. The non-VM Windows 7 test machines handle the test case fine. No other browser chrome tests with the context menu seem affected, so this is all just very strange and indecipherable.

I would like permission to simply disable this browser chrome test on Windows 7 so that we can get the fixes in this bug unblocked and proceed on Quantum Render work. The test would remain enabled for all other Windows test machines. I could file a bug against the partially disabled test so someone with more insight could look into it.
Flags: needinfo?(lsalzman) → needinfo?(jaws)
(In reply to Lee Salzman [:lsalzman] from comment #20)
> (In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on
> intermittent or backout) from comment #19)
> > Backed out for failing browser_bug970746.js on Windows 7 VM:
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > b64554d7181e07f1d8dd7718c211acbf68d2a761
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > 39cb5de383f45c85f3938ebe6cd12e6631bf4773
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/
> > e5912642f421bdd6061be45621fc0d94e675be6f
> > 
> > Push with failures:
> > https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> > inbound&revision=b64554d7181e07f1d8dd7718c211acbf68d2a761&filter-
> > resultStatus=testfailed&filter-resultStatus=busted&filter-
> > resultStatus=exception&filter-resultStatus=retry&filter-
> > resultStatus=usercancel&filter-resultStatus=runnable
> > Failure log:
> > https://treeherder.mozilla.org/logviewer.html#?job_id=112645434&repo=mozilla-
> > inbound
> > 
> > 18:42:16     INFO -  394 INFO TEST-PASS |
> > browser/base/content/test/general/browser_bug970746.js | Menu item text
> > 'Search Google for “I'm some text, …”' contains the correct search terms
> > 'I'm some text, …' -
> > 18:42:16     INFO -  395 INFO TEST-PASS |
> > browser/base/content/test/general/browser_bug970746.js | search context menu
> > item is shown for  '#mixedContent' and selected is 'false' -
> > 18:42:16     INFO -  396 INFO TEST-PASS |
> > browser/base/content/test/general/browser_bug970746.js | search context menu
> > item is shown for  '#partialLink' and selected is 'true' -
> > 18:42:16     INFO -  397 INFO TEST-PASS |
> > browser/base/content/test/general/browser_bug970746.js | Menu item text
> > 'Search Google for “link selection”' contains the correct search terms 'link
> > selection' -
> > 18:42:16     INFO -  Buffered messages finished
> > 18:42:16    ERROR -  398 INFO TEST-UNEXPECTED-FAIL |
> > browser/base/content/test/general/browser_bug970746.js | search context menu
> > item is shown for  '#partialLink' and selected is 'false' - Got true,
> > expected false
> > 18:42:16     INFO -  Stack trace:
> > 18:42:16     INFO -  chrome://mochikit/content/browser-test.js:test_is:967
> > 18:42:16     INFO - 
> > chrome://mochitests/content/browser/browser/base/content/test/general/
> > browser_bug970746.js:null:106
> 
> Jared, it would appear that the test introduced in bug 970746 is failing
> here for a reason that I can't really discern, but only on the Windows 7 VM
> test machine. What is special about this category of test machine is it does
> not support Direct2D, and as of this bug, we would be enabling DirectWrite
> on it. Why this would cause the search-select item in the context menu to
> not show up is beyond me, as on a Windows 7 machine with Direct2D enabled, I
> am not able to reproduce this scenario locally, and there is nothing
> superficially wrong with the test.
> 
> I can't see any obvious reason why changing from GDI fonts to DirectWrite
> fonts would cause an unrelated context menu item to not show up, and only in
> that particular scenario without Direct2D. The non-VM Windows 7 test
> machines handle the test case fine. No other browser chrome tests with the
> context menu seem affected, so this is all just very strange and
> indecipherable.
> 
> I would like permission to simply disable this browser chrome test on
> Windows 7 so that we can get the fixes in this bug unblocked and proceed on
> Quantum Render work. The test would remain enabled for all other Windows
> test machines. I could file a bug against the partially disabled test so
> someone with more insight could look into it.

Rather, I meant to say above "as on a Windows 7 mcahine with Direct2D forcefully disabled"
Separated out this particular change so it could be independently reviewed. See comment 20 for reasoning.
Attachment #8884919 - Flags: review?(jaws)
Comment on attachment 8884919 [details] [diff] [review]
disable test for bug 970746 on Windows 7

Review of attachment 8884919 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser.ini
@@ +309,5 @@
>  # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
>  [browser_bug882977.js]
>  # DO NOT ADD MORE TESTS HERE. USE A TOPICAL DIRECTORY INSTEAD.
>  [browser_bug970746.js]
> +skip-if = (os == "win" && bits == 32) # Disabled on Windows 7 (No D2D but supports DWrite) due to unexplained consequence of bug 1376026

I'm pretty sure we would still want to run this test on other 32-bit Windows platforms.

Instead of making the change here, please add the following at the beginning of the test:

> add_task(function() {
>   if (AppConstants.isPlatformAndVersionAtMost("win", "6.1")) {
>     ok(true, "This test is disabled on Windows 7 (no D2D but supports DWrite) due to unexplained consequence of bug 1376026");
>     return;
>   }
> 
>   ... continue with the rest of the test ...
> }

We don't support anything before Windows 7 now so this check is fine.
Attachment #8884919 - Flags: review?(jaws) → review+
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e738e018fe57
assume DWrite is available on Windows 7 even without the platform update. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5b7263f383d
fix plumbing of DWrite parameters for Skia fonts to not depend on gfxPlatform. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d22cfe83584a
fix tests to assume that Windows 7 always uses DWrite. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a96e12230d
disable test for bug 970746 on Windows 7. r=jaws
Depends on: 1380530
Depends on: 1380759
Depends on: 1383539
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf5603f8b181
follow-up - fix ScaledFontDWrite construct parameter order. r=me
Depends on: 1381668
You need to log in before you can comment on or make changes to this bug.