Bug 81998 – When <img crossorigin> fails the CORS check, we should fire the error event

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

When <img crossorigin> fails the CORS check, we should fire the error event

[Adam Barth]

Created attachment 133395 [details]
Patch

[Adam Barth]

http://code.google.com/p/chromium/issues/detail?id=112170

[Alexey Proskuryakov]

Is this the same as bug 80028?

[Alexey Proskuryakov]

Comment on attachment 133395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133395&action=review

> Source/WebCore/ChangeLog:11
> +        otherwise be because we're firing the event asynchronously, but that
> +        seems like the right thing to do.

Could you please explain why this is the right thing to do? Can CORS failure happen synchronously somehow?

[Adam Barth]

Yes, I forgot we had it on file already. CORS can probably fail synchronously if we get a hit in the MemoryCache.

[Alexey Proskuryakov]

Hmm, I think that memory cache works asynchronously. Not as sure about CORS own cache though. I think that it's worth checking before adding code for asynchronous event dispatch, we usually try to avoid it for events triggered by network.

[Adam Barth]

The memory cache can definitely respond synchronously on a cache hit.

[Nate Chapin]

Comment on attachment 133395 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133395&action=review

> Source/WebCore/loader/ImageLoader.cpp:90
>      , m_firedLoad(true)
> +    , m_firedError(true)
>      , m_imageComplete(true)

Is there any hope of merging any of these booleans? In particular, I would have thought m_firedLoad and m_firedError would be mutually exclusive.

Also, I had never noticed that these are initialized to true. That's counter-intuitive to me, do you know the history behind it?

[Adam Barth]

> Also, I had never noticed that these are initialized to true. That's counter-intuitive to me, do you know the history behind it?

I think they have slightly non-intuitive names.  The "true" state is the quiescent state.  The "false" state means "I wanted to fire this event, but I haven't gotten around to it yet," which has some consequences (like remembering to cancel the event if we don't want it to fire anymore).

Perhaps we should rename m_firedLoad (and friends) to m_loadEventIsPending (and negate the value).  Then the initialization should make sense since "false" would be the quiescent state.

As for merging the bools, they're somewhat separate pieces of state.  I almost was tempted to break them out into a little subobject, but that seemed a bit overkill.

[Nate Chapin]

(In reply to comment #9)
> > Also, I had never noticed that these are initialized to true. That's counter-intuitive to me, do you know the history behind it?
> 
> I think they have slightly non-intuitive names.  The "true" state is the quiescent state.  The "false" state means "I wanted to fire this event, but I haven't gotten around to it yet," which has some consequences (like remembering to cancel the event if we don't want it to fire anymore).
> 
> Perhaps we should rename m_firedLoad (and friends) to m_loadEventIsPending (and negate the value).  Then the initialization should make sense since "false" would be the quiescent state.
> 
> As for merging the bools, they're somewhat separate pieces of state.  I almost was tempted to break them out into a little subobject, but that seemed a bit overkill.

Ok, things make sense now. I'd be in favor of the m_loadEventIsPending naming scheme.

I'm not sure whether they're truly separate pieces of state, though, unless we can have both a pending load event and a pending error event at the same time.

[Adam Barth]

Comment on attachment 133395 [details]
Patch

Would you mind if we changed the names in a follow-up patch.  The issue is that the state is public:

    bool haveFiredBeforeLoadEvent() const { return m_firedBeforeLoad; }
    bool haveFiredLoadEvent() const { return m_firedLoad; }

so the renaming is going to spider a bit.

[Nate Chapin]

Comment on attachment 133395 [details]
Patch

Yeah, I think it's ok. I don't see any reason to hold this patch up because of the existing weird name convention.

[WebKit Review Bot]

Comment on attachment 133395 [details]
Patch

Clearing flags on attachment: 133395

Committed r112190: <http://trac.webkit.org/changeset/112190>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

*** Bug 80028 has been marked as a duplicate of this bug. ***