Closed Bug 1003797 Opened 8 years ago Closed 6 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bobowen, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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
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...
Attachment #8747198 - Flags: feedback?(kyle)
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 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.
Attachment #8747198 - Flags: feedback?(kyle) → feedback?(bzbarsky)
Depends on: 1270181
Comment on attachment 8747198 [details] [diff] [review]
Fire error events for <object> per spec

Good call.  That totally works.
Attachment #8747198 - Flags: feedback?(bzbarsky)
Attachment #8747198 - Attachment is obsolete: true
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8748744 - Flags: review?(kyle) → review+
- 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?)
(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)) {
> 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.
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?
Flags: needinfo?(rlb)
> 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)?
Flags: needinfo?(kyle)
Yup, that should do it.
Flags: needinfo?(kyle)
(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.
Flags: needinfo?(rlb)
Flags: needinfo?(ckerschb)
Flags: needinfo?(annevk)
> 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....
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?
Flags: needinfo?(annevk)
(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.
Flags: needinfo?(ckerschb)
> 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.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a066f45dc90a
Fire error events for <object> per spec.  r=qdot
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/609b20e87865
followup.  Remove a failure annotation from a now-passing test.  r=orange
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.)
My statement above is wrong, determining 2xx requires one request, telling non-2xx and network errors apart requires two.
https://hg.mozilla.org/mozilla-central/rev/a066f45dc90a
https://hg.mozilla.org/mozilla-central/rev/609b20e87865
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.