Closed Bug 696451 Opened 9 years ago Closed 7 years ago
re-load <img> when @crossorigin is updated
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).
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?
Keeping it open for all :).
Assignee: jigneshhk1992 → nobody
Mina, if you're looking for something to do...
My pleasure. I'll look at it tomorrow :-)
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?
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?
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.
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 , 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?  http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#l2715
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. ;)
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
> 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.
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?
Comment on attachment 815088 [details] [diff] [review] Patch v2 Sorry never mind I'll push to try first.
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
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+
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.
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.
Right, I missed that one. Change made.
Attachment #815227 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.