1003797 - Error events are not always fired for <object>s.

Status: RESOLVED FIXED

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

Description

[Bob Owen (:bobowen)]

Steps 4.3 and 4.6 of [1] suggest that we should fire an error event at the <object> before fallback.

We don't appear to be doing this currently:
data:text/html,<object onerror="alert('object onerror')" data="http://people.mozilla.org/nowhere.html"></object>

We should also get a clarification on the spec to see if an error event should be fired when we block because of content policy, view-source URI, etc., as this appears to be a similar situation.

[1] "steps to (re)determine what the object element represents"
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#the-object-element

Comment 1

[Boris Zbarsky [:bzbarsky]]

I was going to try to fix this, so we can get rid of these test failures:

  find testing/web-platform/meta/mixed-content/ -name "*.ini" | grep object-tag

but I'm not entirely sure the spec is sane here.  Filed https://github.com/whatwg/html/issues/1147

There's also the issue at https://github.com/whatwg/html/issues/1142 to consider...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Fire error events for <object> per spec

Comment 3

[Boris Zbarsky [:bzbarsky]]

Kyle, thoughts on this?  I'm having a hard time figuring out where to put this so we make sure we fire the event, but only dire it once. :(

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8747198 [details] [diff] [review]
Fire error events for <object> per spec

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +2506,5 @@
>      mType = eType_Null;
>    }
>  
>    // If we didn't load anything, handle switching to fallback state
>    if (mType == eType_Null) {

I'm not sure there's an instance where we'd have mType == eType_Null that's not an error. Everywhere you've put MaybeFireErrorEvent above is also setting type to null, which means we'll go to the fallback case anyways. So is there a point where we have a null type where we wouldn't want to throw an error? If not, I don't think there's also a case where we'd return early, so you might be able to just put the event firing here? Looks like it would even catch the case that happens in UpdateObjectParameters.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8747198 [details] [diff] [review]
Fire error events for <object> per spec

Good call.  That totally works.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Fire error events for <object> per spec

Comment 7

[John Schoenick [:johns]]

- This code also affects <applet> and <embed>, is that an issue?
- Should an empty <object> fire an error? IIRC it will fall through to type null, but decide not to load any fallback content.

In general type null means we didn't decide to load a plugin/image/document, but that could be because the object isn't fully configured, or because a plugin is disabled or click-to-play (should C2P fire error events? Or should they seem like very-slow-to-load-plugins?)

Comment 8

[John Schoenick [:johns]]

(In reply to John Schoenick [:johns] from comment #7)
> - This code also affects <applet> and <embed>, is that an issue?

Nevermind!

> +  if (thisContent->IsHTMLElement(nsGkAtoms::object)) {

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Should an empty <object> fire an error?

Per spec, as far as I can tell yes.  I'm not sure what should happen in the click-to-play case; the spec doesn't really handle that case.  I would be fine skipping error events when we're a c2p plugin.

Comment 10

[Boris Zbarsky [:bzbarsky]]

This is basically blocked on the security issues discussed in <https://github.com/whatwg/html/issues/1142>.  Richard, do we have someone with the bandwidth to get the spec here sorted out?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I would be fine skipping error events when we're a c2p plugin.

qdot, is that just a matter of conditioning the MaybeFireErrorEvent() call on (fallbackType != eFallbackClickToPlay)?

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Yup, that should do it.

Comment 13

[Richard Barnes [:rbarnes]]

(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #10)
> This is basically blocked on the security issues discussed in
> <https://github.com/whatwg/html/issues/1142>.  Richard, do we have someone
> with the bandwidth to get the spec here sorted out?

Sorry, I'm having a hard time discerning from that issue what the security issues are.  Is the concern that changing the behavior would be exposing more detailed information to JS, e.g., content policy?  I think I agree with your analysis in the Github issue; it doesn't seem like this exposes appreciably more information than not firing load() in these cases.

Perhaps Anne or Christoph have some more insight here.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Is the concern that changing the behavior would be exposing more
> detailed information to JS, e.g., content policy?

Detailed information to JS, yes.  Content policy, less so; information about whether a cross-site URL is 404 or not is a bigger deal.

For <object> this may all be moot anyway due to fallback, since an <object> that has performed fallback has different geometry from one that has not and you can detect that with getBoundingClientRect and company....

Comment 15

[Anne (:annevk)]

So anything that results in a network error (which includes CSP through Fetch) should be fair game to expose.

However, HTTP errors may or may not be expected to leak across origins. E.g., you might be able to use them to go on a fishing expedition on someone's intranet to find interesting projects. Given that <script> exposes them per that thread (except for image MIME types in Chrome, which hopefully other browsers will copy), we may have already lost that. 

That is some of the context. Now, does <object> fallback already for 404 today? It seems weird that <object> would, but <img> would not. And I suspect <object> would not once it is associated with a nested browsing context, right?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Richard Barnes [:rbarnes] from comment #13)
> Perhaps Anne or Christoph have some more insight here.

It seems Anne is on this one - clearing my needinfo.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Now, does <object> fallback already for 404 today?

Yes.  data:text/html,<object data="http://example.com/gofish">I am fallback</object>

> It seems weird that <object> would, but <img> would not.

<img> doesn't only if the 404 has an actual image type, right?  I believe that was done for compat reasons only....

> And I suspect <object> would not once it is associated with a nested browsing context, right?

It depends.  If you navigate the browsing context directly via setting location or whatnot, then no.  If you navigate by changing @data on the <object> then yes.

Anyway, I've convinced myself that all browsers leak everything through <object> already and so we should just go ahead and check this in.  If/when the spec actually gets sorted out, we can change our implementation as needed.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a066f45dc90a
Fire error events for <object> per spec.  r=qdot

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/609b20e87865
followup.  Remove a failure annotation from a now-passing test.  r=orange

Comment 20

[Anne (:annevk)]

Yeah, it seems like we effectively leak 2xx even for "no-cors" (though to determine 2xx requires two requests at the moment, one through an API that doesn't treat non-2xx and network errors the same, and one that does). Yay same-origin policy exceptions.

(To be clear, unless there's a network error, <img> looks for image/svg+xml, and sniffs otherwise. If sniffing doesn't return an image MIME type, it's handled as if there's a network error. So the status of the response never really comes into play.)

Comment 21

[Anne (:annevk)]

My statement above is wrong, determining 2xx requires one request, telling non-2xx and network errors apart requires two.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a066f45dc90a
https://hg.mozilla.org/mozilla-central/rev/609b20e87865