Closed Bug 1418930 Opened 2 years ago Closed 2 years ago

Implement CORS-enabled fetch for shape images

Categories

(Core :: Layout: Floats, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: TYLin, Assigned: bradwerth)

References

Details

Attachments

(7 files)

Per spec, we need to implement CORS-enabled fetch ("Anonymous" mode) for shape image URLs.

https://drafts.csswg.org/css-shapes/#valdef-shape-outside-image
Priority: -- → P3
See Also: → 1418470
Assignee: nobody → tlin
Status: NEW → ASSIGNED
I'll try to move this forward.
Assignee: aethanyc → bwerth
Attachment #8945645 - Flags: review?(emilio)
Attachment #8945646 - Flags: review?(emilio)
Attachment #8945647 - Flags: review?(emilio)
Attachment #8945648 - Flags: review?(emilio)
Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

https://reviewboard.mozilla.org/r/215778/#review222280

r=me, with the flag double-checked and the enum class, thanks!

::: layout/style/ImageLoader.h:68
(Diff revision 4)
>    // in because this can be called during presentation destruction after the
>    // presshell pointer on the document has been cleared.
>    void ClearFrames(nsPresContext* aPresContext);
>  
>    void LoadImage(nsIURI* aURI, nsIPrincipal* aPrincipal, nsIURI* aReferrer,
> -                 Image* aCSSValue);
> +                 Image* aCSSValue, bool aUseAnonymousCORS);

Can we make this an enum class (even if it's just a Yes / No variant) to make this clear on the caller?

```
enum class UseAnonymousCORS {
    Yes,
    No
};
```

::: layout/style/ImageLoader.cpp:260
(Diff revision 4)
>    if (!aURI) {
>      return;
>    }
>  
> +  int32_t loadFlags = nsIRequest::LOAD_NORMAL;
> +  if (MOZ_UNLIKELY(aUseAnonymousCORS)) {

This looks legit, but please double-check this with someone more familiar than I with the networking code, to double check that we don't need anything else.

Also since LOAD_NORMAL is 0, this may look cleaner with the ternary operator (or not, not sure, your call :P)
Attachment #8945645 - Flags: review?(emilio) → review+
Comment on attachment 8945646 [details]
Bug 1418930 Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading.

https://reviewboard.mozilla.org/r/215780/#review222282

::: layout/style/nsCSSValue.h:207
(Diff revision 4)
>  protected:
>    // Only used by ImageValue.  Declared up here because otherwise bindgen gets
>    // confused by the non-standard-layout packing of the variable up into
>    // URLValueData.
>    bool mLoadedImage = false;
> +  bool mUseAnonymousCORS = false;

Here we can use the enum class defined before and all that :)
Attachment #8945646 - Flags: review?(emilio) → review+
Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

https://reviewboard.mozilla.org/r/215782/#review222284

What do you think about that? I'd really prefer to reduce mutation in our style structs as much as possible, even though here we're guaranteed to be on the main thread.

::: commit-message-4d588:1
(Diff revision 4)
> +Bug 1418930 Part 3: Update nsStyleStruct::FinishStyle to add flag for anonymous CORS to shape-outside images.

nit: `nsStyleDisplay::FinishStyle`.

::: layout/style/nsStyleStruct.cpp:3716
(Diff revision 4)
>    MOZ_ASSERT(aPresContext->StyleSet()->IsServo());
>  
>    if (mShapeOutside.GetType() == StyleShapeSourceType::Image) {
>      const UniquePtr<nsStyleImage>& shapeImage = mShapeOutside.GetShapeImage();
>      if (shapeImage) {
> +      shapeImage->GetImageRequest()->GetImageValue()->SetUseAnonymousCORS(true);

Hmm... It's really weird to put this here. Why can't it be done when constructing the ImageValue from style? I think that'd be cleaner. It requires modifying the ImageValue constructor, but I think it'd be nicer.
Attachment #8945647 - Flags: review?(emilio)
Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

https://reviewboard.mozilla.org/r/215784/#review222286

Is there any reason this can't be written as a WPT test? I think we should default to WPT for new tests unless there's a strong reason not to.

::: layout/style/test/test_shape_outside_with_cors.html:7
(Diff revision 4)
> +<html>
> +<head>
> +<meta charset="utf-8">
> +<title>CSS Test: shape-outside with a CORS violation</title>
> +<link rel="author" title="Brad Werth" href="mailto:bwerth@mozilla.com"/>
> +<link rel="help" href="http://www.w3.org/TR/css-shapes-1/#shape-outside-property"/>

nit: let's link to the csswg draft instead:

https://drafts.csswg.org/css-shapes/

::: layout/style/test/test_shape_outside_with_cors.html:8
(Diff revision 4)
> +<head>
> +<meta charset="utf-8">
> +<title>CSS Test: shape-outside with a CORS violation</title>
> +<link rel="author" title="Brad Werth" href="mailto:bwerth@mozilla.com"/>
> +<link rel="help" href="http://www.w3.org/TR/css-shapes-1/#shape-outside-property"/>
> +

nit: stray newline?

::: layout/style/test/test_shape_outside_with_cors.html:9
(Diff revision 4)
> +<meta charset="utf-8">
> +<title>CSS Test: shape-outside with a CORS violation</title>
> +<link rel="author" title="Brad Werth" href="mailto:bwerth@mozilla.com"/>
> +<link rel="help" href="http://www.w3.org/TR/css-shapes-1/#shape-outside-property"/>
> +
> +<script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>↩

nit: no need for `type=`

::: layout/style/test/test_shape_outside_with_cors.html:10
(Diff revision 4)
> +<title>CSS Test: shape-outside with a CORS violation</title>
> +<link rel="author" title="Brad Werth" href="mailto:bwerth@mozilla.com"/>
> +<link rel="help" href="http://www.w3.org/TR/css-shapes-1/#shape-outside-property"/>
> +
> +<script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>↩
> +<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />

nit: no need for `type=`

::: layout/style/test/test_shape_outside_with_cors.html:12
(Diff revision 4)
> +<link rel="help" href="http://www.w3.org/TR/css-shapes-1/#shape-outside-property"/>
> +
> +<script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>↩
> +<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +
> +<style type="text/css">

nit: no need for `type=`

::: layout/style/test/test_shape_outside_with_cors.html:45
(Diff revision 4)
> +
> +function runTests() {
> +  // Test 1: Confirm that a shape-outside with a same-origin URL works.
> +  let divAccept = document.getElementById("accept");
> +  let divAcceptChild = divAccept.children[0];
> +  divAccept.style.shapeOutside = 'url("http://mochi.test:8888/dom/html/test/image.png")';

This is not guaranteed to work at all I think, why would the image loading be synchronous?

You should probably specify them on the stylesheet, so the loads block the document load event.
Attachment #8945648 - Flags: review?(emilio)
Attachment #8945645 - Flags: review?(bzbarsky)
Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

https://reviewboard.mozilla.org/r/215778/#review222356

::: layout/style/ImageLoader.cpp:260
(Diff revision 4)
>    if (!aURI) {
>      return;
>    }
>  
> +  int32_t loadFlags = nsIRequest::LOAD_NORMAL;
> +  if (MOZ_UNLIKELY(aUseAnonymousCORS)) {

I'm relying on https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/image/imgLoader.cpp#2270 to indicate that CORS mode is already triggerable with a simple flag. I'll add bz as reviewer to confirm (he reviewed parts of Bug 664299 which added that code).

Regardling LOAD_NORMAL and additional flags, I'll leave it as a bitwise OR, since now that we're using the CORSMode enum, it's possible for there to be other policies added later (such as CORS_USE_CREDENTIALS) where the bitwise will flow better.
Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

https://reviewboard.mozilla.org/r/215782/#review222284

> Hmm... It's really weird to put this here. Why can't it be done when constructing the ImageValue from style? I think that'd be cleaner. It requires modifying the ImageValue constructor, but I think it'd be nicer.

Yes, it's weird. I'm prepping a new version of the patch that moves this to the shape-outside case of nsRuleNode::ComputeDisplayData(). I'll also try to make it more of a parameter-passing call instead of a drill-through call.
Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

https://reviewboard.mozilla.org/r/215784/#review222534
Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

https://reviewboard.mozilla.org/r/215784/#review222286

One factor for this bug is that the feature is not yet enabled; we're trying to assure ourselves that the CORS mode is respected before we land the bug that enables the feature. There are already lots of WPT tests of shape-outside that we'll turn on once we land the feature patch itself (blocked by this bug). To test just the CORS rejection, I thought it would be useful to set a todo in a new mochitest that we want to see unexepectedly pass when the feature is landed.

> This is not guaranteed to work at all I think, why would the image loading be synchronous?
> 
> You should probably specify them on the stylesheet, so the loads block the document load event.

Good point. I was hoping to avoid an embedded iframe for this test, but it may be unavoidable. We're challenged because a pref has to be turned on, and if we use SpecialPowers to do that, we have to load the shape-outside styled content afterwards. I'll rebuild the test around an embedded iframe.
Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

https://reviewboard.mozilla.org/r/215778/#review222572

::: layout/style/ImageLoader.cpp:260
(Diff revision 5)
>    if (!aURI) {
>      return;
>    }
>  
> +  int32_t loadFlags = nsIRequest::LOAD_NORMAL;
> +  if (MOZ_UNLIKELY(aCorsMode == CORSMode::CORS_ANONYMOUS)) {

Why do we need this MOZ_UNLIKELY?

Do we really want to load CORS_USE_CREDENTIALS as LOAD_NORMAL?  Seems like we'd want to convert that to imgILoader::LOAD_CORS_USE_CREDENTIALS.

Also, I'd really prefer it if we had some utility method here instead of adding a third copy of this "map the CORS flags over to the imagelib flag" code.
Attachment #8945645 - Flags: review?(bzbarsky) → review+
(In reply to Brad Werth [:bradwerth] from comment #29)
> Comment on attachment 8945647 [details]
> Bug 1418930 Part 3: Set CORS Mode for ImageValues created by shape-outside
> declarations.
> 
> https://reviewboard.mozilla.org/r/215782/#review222284
> 
> > Hmm... It's really weird to put this here. Why can't it be done when constructing the ImageValue from style? I think that'd be cleaner. It requires modifying the ImageValue constructor, but I think it'd be nicer.
> 
> Yes, it's weird. I'm prepping a new version of the patch that moves this to
> the shape-outside case of nsRuleNode::ComputeDisplayData(). I'll also try to
> make it more of a parameter-passing call instead of a drill-through call.

I just realized an advantage of putting this call in this weird place. If it's here, it applies to both Gecko and Stylo parsed styles. If the call is moved back to the point of creation of the ImageValue, we have to change it in at least two places (Gecko in nsRuleNode::ComputeDisplayData, and Stylo in basic_shape::parse or thereabouts). I'm not confident in my ability to produce the servo changes quickly in a way that isn't totally gross. Because this bug is a blocker for landing shape-outside, which is a target for 60, I'd like to land the ugly version first and file a bug for a "CORS Mode per property" cleanup that would move this earlier for both Gecko and Servo. Emilio, is that acceptable?
Flags: needinfo?(emilio)
Comment on attachment 8945645 [details]
Bug 1418930 Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers.

https://reviewboard.mozilla.org/r/215778/#review222572

> Why do we need this MOZ_UNLIKELY?
> 
> Do we really want to load CORS_USE_CREDENTIALS as LOAD_NORMAL?  Seems like we'd want to convert that to imgILoader::LOAD_CORS_USE_CREDENTIALS.
> 
> Also, I'd really prefer it if we had some utility method here instead of adding a third copy of this "map the CORS flags over to the imagelib flag" code.

Utility method added as a new Part 1 of this patch that addresses all these issues.
Attachment #8945648 - Flags: review?(emilio) → review?(bzbarsky)
Attachment #8945647 - Flags: review?(emilio)
Attachment #8947248 - Flags: review?(emilio)
Comment on attachment 8945648 [details]
Bug 1418930 Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags.

https://reviewboard.mozilla.org/r/215784/#review222854

r=me
Attachment #8945648 - Flags: review?(bzbarsky) → review+
Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

https://reviewboard.mozilla.org/r/215782/#review222974

::: layout/style/nsStyleStruct.cpp:3735
(Diff revision 6)
>    MOZ_ASSERT(aPresContext->StyleSet()->IsServo());
>  
>    if (mShapeOutside.GetType() == StyleShapeSourceType::Image) {
>      const UniquePtr<nsStyleImage>& shapeImage = mShapeOutside.GetShapeImage();
>      if (shapeImage) {
> +      shapeImage->GetImageRequest()->GetImageValue()->SetCORSMode(

Mutations are bad and this is ugly. But I'd still like to land the code this way since it works for both Gecko and Stylo. If it's acceptable, I'll open a new bug to move the CORS Mode changes to the point of creation of the ImageValue in both Gecko and Stylo, and put a bug comment here.
Comment on attachment 8945647 [details]
Bug 1418930 Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images.

https://reviewboard.mozilla.org/r/215782/#review222540

r=me, reluctantly :)

As I said I'd prefer fixing it instead of adding it here, but your call.

::: layout/style/nsStyleStruct.cpp:3735
(Diff revision 6)
>    MOZ_ASSERT(aPresContext->StyleSet()->IsServo());
>  
>    if (mShapeOutside.GetType() == StyleShapeSourceType::Image) {
>      const UniquePtr<nsStyleImage>& shapeImage = mShapeOutside.GetShapeImage();
>      if (shapeImage) {
> +      shapeImage->GetImageRequest()->GetImageValue()->SetCORSMode(

I think this shouldn't be hard to fix. Image values in Servo are constructed at parse-time in `Url::build_image_value`. It's a matter of making a `CorsMode` arrive there.

Which itself it's a matter of moving the parsing of `Image` to something like `parse_with_cors_mode`, and making a `struct ShapeImage(pub Image)` (or `CorsImage`? I'm terrible at naming) using it in `components/style/values/specified/basic_shape.rs`, calling it.

I'd appreciate fixing it, honestly this feels like a huge hack. But if you don't, please file a bug, and do reference it from a comment here.
Attachment #8945647 - Flags: review+
Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

https://reviewboard.mozilla.org/r/216998/#review223000

I'm not sure there's much value in landing the test if as you mentioned there are WPT tests for this, and this test is marked as "todo" since this still hasn't landed. Mind elaborating?

Anyway I guess more tests are fine, just want to know the reasoning before stamping.

::: layout/style/test/test_shape_outside_CORS.html:16
(Diff revision 1)
> +<script>
> +SimpleTest.waitForExplicitFinish();
> +
> +// Set a pref that we'll need before we load contents into the iframe.
> +const oldShapeOutsidePref = SpecialPowers.getBoolPref("layout.css.shape-outside.enabled");
> +SpecialPowers.setBoolPref("layout.css.shape-outside.enabled", true);

There's no other `setBoolPref` call in layout/style/test, other tests use pushPrefEnv. What happens if the test times out (not sure, just asking)?
Attachment #8947248 - Flags: review?(emilio)
Flags: needinfo?(emilio)
Blocks: 1434963
Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

https://reviewboard.mozilla.org/r/216998/#review223000

The WPT tests might not check the CORS mode behavior. There are a lot of shape-outside tests, but a CORS mode check is necessarily either browser dependent (check the error text that shows up in the browser console) and therefore not in WPT or it tests the absence of changes from the feature, as this test does. So landing this is a good sanity-check for when we turn on the shape-outside feature, in case WPT doesn't attempt to check CORS.

> There's no other `setBoolPref` call in layout/style/test, other tests use pushPrefEnv. What happens if the test times out (not sure, just asking)?

I changed it to use pushPrefEnv/popPrefEnv. I don't know if it makes the pref state more resilient in a test timeout, but it can't hurt.
Comment on attachment 8947248 [details]
Bug 1418930 Part 6: Add a test of shape-outside with and without a CORS violation.

https://reviewboard.mozilla.org/r/216998/#review223152

r=me with the popPrefEnv bit fixed / removed.

::: layout/style/test/file_shape_outside_CORS.html:37
(Diff revision 3)
> +  window.parent.postMessage({
> +    "result": (divAllowChild.getBoundingClientRect().left != divAllow.getBoundingClientRect().left),
> +    "message": "Test 1: Child is at different left offset from parent (shape-outside was allowed).",
> +    "todo": true,
> +    },
> +    "http://mochi.test:8888");

it'd be slightly nice to not hardcode the testing domain, but not sure if it's doable, it's not a big deal either way.

::: layout/style/test/test_shape_outside_CORS.html:21
(Diff revision 3)
> +{
> +  if (event.data.done) {
> +    // Remove ourself as an event listener, just to be thorough.
> +    window.removeEventListener("message", receiveMessage);
> +    // Undo our meddling in preferences, then finish the test.
> +    SpecialPowers.popPrefEnv(SimpleTest.finish());

Oh, I was totally assuming that pushPrefEnv popped the preference itself when the callback returned... But apparently the caller is responsible to call popPrefEnv, and we basically never do.

So the point in using it I think is the fact that the test runner calls flushPrefEnv:

  https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/testing/mochitest/tests/SimpleTest/TestRunner.js#645
  
which means that you don't really need to `popPrefEnv` manually here.

That being said, if you want to do so you probably want to do:

```
popPrefEnv(SimpleTest.finish)
```

Since popPrefEnv gets a `callback` parameter, not a return value, and I think calling popPrefEnv _after_ SimpleTest.finish is somewhat racy (someone could've pushed a pref already in other test).

But per the avobe the prefs are auto-cleaned-up, so just `SimpleTest.finish()` should work.

And that also means that the win of using `pushPrefEnv` vs `setFooPref` is that if the test fails / throws / times out, the runner manages to restore the prefs, instead of affecting the following tests. I guess I learned something today, thanks! :)
Attachment #8947248 - Flags: review?(emilio) → review+
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/27eb8c7f8025
Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags. r=bz
https://hg.mozilla.org/integration/autoland/rev/105c81284dba
Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers. r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/8008a54aba9a
Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading. r=emilio
https://hg.mozilla.org/integration/autoland/rev/b39948ba905b
Part 4: Update nsStyleStruct::FinishStyle to set CORS mode for shape-outside images. r=emilio
https://hg.mozilla.org/integration/autoland/rev/c0f673033e66
Part 5: Add a test of shape-outside with and without a CORS violation. r=emilio
Attachment #8948446 - Flags: review?(emilio)
Comment on attachment 8948446 [details]
Bug 1418930 Part 7: Change a web platform test to PASS.

https://reviewboard.mozilla.org/r/217886/#review223710

r=me, thanks!

::: testing/web-platform/meta/service-workers/service-worker/fetch-request-css-images.https.html.ini:4
(Diff revision 1)
>  [fetch-request-css-images.https.html]
>    expected: TIMEOUT
>    [Verify FetchEvent for css image (shapeOutside).]
> -    expected: FAIL
> +    expected: PASS

Just remove this line and the line above it, tests default to `PASS`.
Attachment #8948446 - Flags: review?(emilio) → review+
In order to pass testing/web-platform/meta/service-workers/service-worker/fetch-request-css-images.https.html on Gecko, I had to implement the Gecko part of the fix for Bug 1434963. The Stylo fix remains (and Bug 1434963 remains open).
Attachment #8948859 - Flags: review?(emilio)
Comment on attachment 8948859 [details]
Bug 1418930 Part 4: Define a CSS property flag to accompany the image preload flag -- to also use anonymous CORS.

https://reviewboard.mozilla.org/r/218246/#review224162

looks good, thanks! r=me
Attachment #8948859 - Flags: review?(emilio) → review+
Attempting to land again.
Flags: needinfo?(bwerth)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d45873f6dcc4
Part 1: Define and use a method nsContentUtils::CORSModeToLoadImageFlags to standardize conversion of CORS modes to load image flags. r=bz
https://hg.mozilla.org/integration/autoland/rev/cac787a13132
Part 2: Extend ImageLoader with a parameter to add anonymous CORS headers. r=bz,emilio
https://hg.mozilla.org/integration/autoland/rev/b1868b7597e2
Part 3: Update css::ImageValue to carry a parameter that indicates anonymous CORS headers should be used during loading. r=emilio
https://hg.mozilla.org/integration/autoland/rev/9c080d88ee0c
Part 4: Define a CSS property flag to accompany the image preload flag -- to also use anonymous CORS. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e187bbe58b54
Part 5: Update nsStyleStruct::FinishStyle and nsRuleNode::ComputeDisplayData to set CORS mode for shape-outside images. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8554668651cc
Part 6: Add a test of shape-outside with and without a CORS violation. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7a8fceb25ceb
Part 7: Change a web platform test to PASS. r=emilio
You need to log in before you can comment on or make changes to this bug.