855615 - nsIDOMHTMLMediaElement.idl documentation / tests incorrect

Status: RESOLVED INVALID

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

Description

[David Chan [:dchan]]

The idl for nsIDOMHTMLElement states that setting a mozAudioChannelType without the proper permission throws. [1] However this is not the case. As a result the test code has been erroneously succeeding. [2] You can verify this by removing the try-catch in the test and the test will still pass. This was tested with a recent emulator build, < 1 week old.

Code flow for setting mozaudiochannel / mozAudioChannelType
1. ParseAttribute is called for the atom mozaudio channel [3]
2. The attribute is looked up against the enum table kMozAudioChannelAttributeTable, defaulting to AUDIO_CHANNEL_NORMAL if not found. This explains why the test was resetting mozAudioChannelType to "normal"
3. CheckAudioChannelPermissions() is called if the audioChannelType is different and the decoder is setup
4. If permission is granted, then mAudioChannelType is updated.
5. true is returned

The above workflow can lead to an inconsistent state. mAudioChannelType may not correspond to mozAudioChannelType. In my testing, I see mAudioChannelType remaining at 0 (AUDIO_CHANNEL_NORMAL) while mozAudioChannelType is something else.


To repeat, permissions are still checked. However any code that relies on the setting of mozAudioChannelType to throw will fail.


[1] - http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/html/nsIDOMHTMLMediaElement.idl#117
[2] - http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_mozaudiochannel.html?force=1
[3] - http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#2135

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The documented behaviour not reasonable if we implement this as reflecting a content attribute, as was done in bug 817043. We should update the test and the documentation.

Comment 2

[Wayne Chang [:wchang]]

Hi David, can you please explain the user impact for this so we can understand the severity and decide whether it should be a blocker for leo?

Comment 3

[David Chan [:dchan]]

(In reply to Wayne Chang [:wchang] from comment #2)
> Hi David, can you please explain the user impact for this so we can
> understand the severity and decide whether it should be a blocker for leo?

Hi Wayne,

This particular bug makes it so that content can't discover if the audio-channel for an element has changed since the attribute is always updated. Previously, setting a disallowed audio channel level would throw an exception and not update the mozChannel attribute. Code that relies on the mozChannel attribute being accurate may fail because of this bug. Chrome code should be okay if it listens for the audio-channel-changed event.

The user impact is likely minimal. The bug doesn't change how the underlying permissions work for audio-channel.

Comment 4

[Jason Smith [:jsmith]]

(In reply to David Chan [:dchan] from comment #3)
> (In reply to Wayne Chang [:wchang] from comment #2)
> > Hi David, can you please explain the user impact for this so we can
> > understand the severity and decide whether it should be a blocker for leo?
> 
> Hi Wayne,
> 
> This particular bug makes it so that content can't discover if the
> audio-channel for an element has changed since the attribute is always
> updated. Previously, setting a disallowed audio channel level would throw an
> exception and not update the mozChannel attribute. Code that relies on the
> mozChannel attribute being accurate may fail because of this bug. Chrome
> code should be okay if it listens for the audio-channel-changed event.
> 
> The user impact is likely minimal. The bug doesn't change how the underlying
> permissions work for audio-channel.

FWIW, that probably implies that this is not a blocker then if the user impact is minimal.

Comment 5

[David Chan [:dchan]]

I agree that this isn't a blocker. An app with the proper permissions will still work. It's the misbehaving apps that would have problems. Perhaps we can update the documentation in the meantime to say the property won't throw.

It does make testing more difficult, but I don't believe we've blocked on that before.

Comment 6

[Dietrich Ayala (:dietrich)]

This patch would have an easy approval path.

Comment 7

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

Invalidating, as we're now removing nsIDOMHTMLMediaElement in Bug 1407040. Also, it looks like the types referred to in here are no longer used in HTMLMediaElement.