Closed Bug 696451 Opened 9 years ago Closed 7 years ago

re-load <img> when @crossorigin is updated

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: sicking, Assigned: almasry.mina)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 8 obsolete files)

The following two code paths should do the same thing:

img = new Image();
img.crossOrigin = "anonymous";
img.src = someurl;

vs.

img = new Image();
img.src = someurl;
img.crossOrigin = "anonymous";

In other words, if someone sets the crossorigin attribute on an <img>, we should re-fetch the image using the new security policy.

This might even be a problem with markup like:

<img src="..." crossorigin>
vs.
<img crossorigin src="...">

(I forget if we parse attribute front-to-back or back-to-front, so don't know which of the above is an issue).
OS: Mac OS X → All
Hardware: x86 → All
Attached patch Patch (obsolete) — Splinter Review
Attachment #593324 - Flags: feedback?(jonas)
Ignore indentation for this patch.
Comment on attachment 593324 [details] [diff] [review]
Patch

Looks good. Though it's a bummer to have attribute-handling logic spread out over SetAttr and AfterSetAttr.

Would you mind also moving the logic that's in SetAttr/UnsetAttr to your new AfterSetAttr function?
Attachment #593324 - Flags: feedback?(jonas) → feedback+
Were you planning to finish this work, Jignesh?
Assignee: nobody → jigneshhk1992
For few more days. If someone wants to take it feel free else I will complete it.
Jignesh, are you still planning to work on this?
Flags: needinfo?(jigneshhk1992)
Keeping it open for all :).
Assignee: jigneshhk1992 → nobody
Flags: needinfo?(jigneshhk1992)
Mina, if you're looking for something to do...
Flags: needinfo?(almasry.mina)
My pleasure. I'll look at it tomorrow :-)
Flags: needinfo?(almasry.mina)
Assignee: nobody → almasry.mina
Attached patch Patch (obsolete) — Splinter Review
Here is a patch. It's actually almost the same as jhk's. I couldn't move the block in in unSetAttr because that caused test failures:

https://tbpl.mozilla.org/?tree=Try&rev=41faabd3ae6a

And I couldn't move the block in SetAttr because there is a comment on top of the block that says it should run before SetAttr is called.

Is there anything else you want done?
Attachment #593324 - Attachment is obsolete: true
Attachment #792296 - Flags: review?(jonas)
Comment on attachment 792296 [details] [diff] [review]
Patch

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

I believe bz has been messing with this more than I have
Attachment #792296 - Flags: review?(jonas) → review?(bzbarsky)
Comment on attachment 792296 [details] [diff] [review]
Patch

Hrm.  That comment in SetAttr dates back to bug 83774.

I am having a hard time remembering why SetAttr kicked off a reflow directly back then (!), but it sure shouldn't now.  It should be safe to move the LoadImage to AfterSetAttr.

And moving the UnsetAttr code to AfterSetAttr is fine too, but you have to condition it on the attr being _unset_.  Your test run failed because that code was running for attr _sets_, too, and canceling loads that should not have gotten canceled.

That said, the biggest problem with this patch is that MaybeLoadImage will not do a load if the URI has not changed, which seems wrong in this case.  Please add tests to verify that the patch is in fact doing what it should?
Attachment #792296 - Flags: review?(bzbarsky) → review-
Oh, I meant to say:  Just putting the new thing in AfterSetAttr seems like the safe thing to do in this case...
Mina, are you still going to have time to wrap this up?
Flags: needinfo?(almasry.mina)
Yes. Sorry for the delay but I'm back on it. If I'm holding anyone back with the delay feel free to assign it to someone else, but I'm back on it anyway.
Flags: needinfo?(almasry.mina)
Keywords: dev-doc-needed
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #792296 - Attachment is obsolete: true
Attachment #814373 - Flags: review?
Attachment #814373 - Flags: review?
Attached patch Test and debug prints (obsolete) — Splinter Review
So here is the patch and a test with some debug prints.

I was hoping in the test that "load" event would fire twice, once for set of .src and another for set of .crossorigin, but that doesn't happen, it only gets fired for set of .src

But I followed the code path of the set of .crossOrigin all the way to here [1], and the image seems to be reloaded with this patch just as we want, it's just that the load event doesn't fire.

Is this enough for this bug or do you want me to find out why the load event doesn't fire?

[1] http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#l2715
Flags: needinfo?(bzbarsky)
You only get one load, because the old load gets canceled.

To write a test for this bug, you want to check that the resulting image was actually loaded with the right CORS mode.  For example, do the load cross-origin, then onload paint the image to a canvas and see whether you can read back from the canvas.  Without the fix you should not be able to, but with the fix you should be able to....

Or, load cross-origin but have the server not send any access-control-allow-origin.  Without this patch, that should give you a load event, but with the patch you should get an error event, since the image won't be allowed to load.

Or test both.  ;)
Flags: needinfo?(bzbarsky)
Attached patch Test (obsolete) — Splinter Review
Attachment #814373 - Attachment is obsolete: true
Attachment #814391 - Attachment is obsolete: true
Attached patch Refactor Patch (obsolete) — Splinter Review
I tried your first suggestion (mainly because I'm not sure how to do the second in a mochitest) and it turns out it works correctly with or without a fix. I've attached here a test case that checks for that and a patch that only refactors the code that's already there and corrects the comment. Feel free to any of these if you want them checked in or mark this bug resolved.

Try for both patches: https://tbpl.mozilla.org/?tree=Try&rev=ba7012f74919
Flags: needinfo?(bzbarsky)
> and it turns out it works correctly with or without a fix

Then it's not the right test.

If you're talking about attachment 814706 [details] [diff] [review], then it's not testing my first suggestion, because it's not doing a cross-origin load.
Flags: needinfo?(bzbarsky)
Attached patch Patch v2 (obsolete) — Splinter Review
Right, I didn't have my head wrapped around what cross-origin actually means. Here is a refactor + fix + the 2 tests you wanted. Did I get it right?
Attachment #814706 - Attachment is obsolete: true
Attachment #814707 - Attachment is obsolete: true
Attachment #815088 - Flags: review?(bzbarsky)
Comment on attachment 815088 [details] [diff] [review]
Patch v2

Sorry never mind I'll push to try first.
Attachment #815088 - Flags: review?(bzbarsky)
Comment on attachment 815088 [details] [diff] [review]
Patch v2

Looks like the try is fine (windows and osx slaves disconnected en masse earlier today). Review please.

https://tbpl.mozilla.org/?tree=Try&rev=6a6e36cc5630
Attachment #815088 - Flags: review?(bzbarsky)
Comment on attachment 815088 [details] [diff] [review]
Patch v2

>+    LoadImage(uri, true, aNotify);

Please document why the second argument is true.

>+  // If we plan to call LoadImage, we want to do it first so that the image load

This should probably move up above the crossorigin attr handling, since it applies to that too.

>+    nsAutoString uri;
>+    GetAttr(kNameSpaceID_None, nsGkAtoms::src, uri);
>+    LoadImage(uri, true, aNotify);

Why?  Why not:

  LoadImage(*aValue, true, aNotify);

and skip the extra GetAttr?

The test looks great.  r=me with the above fixed.
Attachment #815088 - Flags: review?(bzbarsky) → review+
Attached patch Patch v3 (obsolete) — Splinter Review
Above fixed and marking checkin needed.

LoadImage(*aValue, ...) didn't work btw, but this did:

nsAutoString uri;
aValue->ToString(uri);
LoadImage(uri, ...);

Let me know if that's not what you want.
Attachment #815088 - Attachment is obsolete: true
Thanks bz!
Keywords: checkin-needed
Er, right, aValue is an nsAttrValue nowadays...

You should be able to do:

  LoadImage(aValue->GetStringValue(), ...);

since you know it's a string in this case.
Keywords: checkin-needed
Attached patch Patch v4Splinter Review
Right, I missed that one. Change made.
Attachment #815227 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/41e7e41afe1f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 980243
You need to log in before you can comment on or make changes to this bug.