478957 - Audit events when loading local files from another domain

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

When audio/video elements try to load a resource, but access to that resource is restricted by load policy, we should not send any events or make state changes which tell the page that the restricted resource exists.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - mochitest to detect event/state leak

This patch adds a mochitest that attempts video/audio loads of local sources from another domain while listening on all specified events and polling every 1ms to detect state changes. This test checks that we have the correct events in the correct order, that we don't leak state, and also checks that pages served from another domain can't load local media.

This patch is based on top of the patch for bug 465458, which updates the load algorithm to match the current spec. The mochitest passes on Windows and Linux (provided the patch for bug 465458 applied).

It's possible that there are weird edge cases that this mochitest misses. I will still go through and audit the code manually, and maybe others want to as well, but this test gives us some confidence and will allows us to detect regressions.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is cool.

typo: "durationchane"

You should probably add a test for cross-origin HTTP loads of non-media content.

As a separate issue, you could think about what, if anything we might want to hide for cross-origin media loads.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

As Patch v1, with spelling error fixed and HTTP cross-domain non-media load test case added.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3 - v2 + check currentSrc

When reviewing the code, I realized I'd not checked the currentSrc attribute in my mochitest, so I've updated the test to check that. I also expanded a comment about a setTimeout() which Chris Double asked me about.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Result of code audit:

* All state changes to networkState > LOADING or readyState > HAVE_NOTHING are initiated by the decoder.
* Decoders are only constructed if the content policy does not veto the load, or if the channel has an appropriate mime type.
* The only other events sent in nsHTMLMediaElement are loadstart, error, emptied and play. These can be sent regardless of networkState or readyState, and you can't tell the difference between a 404 and a vetoed load with these events.

So I think we're ok.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

The patch for bug 465458 must land before this one can.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

this is ready to land now I guess

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4 - unbitrotten

As patch v3, but unbitrotten. This patch is based on top of bug 479711 and bug 479859. As such, the test is changed slightly, we now assume that networkState can be either <= NETWORK_LOADING, or NETWORK_NO_SOURCE (previously <= NETWORK_LOADING, NETWORK_NO_SOURCE is new). We also now never expect to receive an 'emptied' event, since now this only happens when we get an error during load, or we abort a load because a new one has started.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v5 - with logging disabled

I left some "dump" debug calls commented in by mistake, turning them off again.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/5e2e306f2803

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/dc3793b2c3e2