793315 - Fix mozGetMetadata with single-tag streams

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

The error handling code in nsHTMLMediaElement::MozGetMetadata is wrong.

nsDataHashtable::EnumerateRead returns the number of items read, not the final PLDHashOperator, so checking the return value against PL_HASH_STOP just means we error out on single-tag streams.

Comment 1

[Ralph Giles (:rillian)]

Attachment: Part 1 - propagate errors correctly

Comment 2

[Ralph Giles (:rillian)]

Attachment: Part 2 - verify the fix in the test suite

Well, that was embarrassing.

Comment 3

[Ralph Giles (:rillian)]

Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=7f4681fb63b9

Comment 4

[Ralph Giles (:rillian)]

Comment on attachment 663569 [details] [diff] [review]
Part 1 - propagate errors correctly

Try fails on optimized builds because the 'count' return value is only used in the LOG statement, and so generates an unused-variable warning on optimized builds.

Comment 5

[Ralph Giles (:rillian)]

Attachment: Part 1 - propagate errors correctly

Just remove the offending log message. It's not important.

Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=08d4c3b0c20d. I don't understand the build failures with corresponding successful mochitests.

Comment 6

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

Comment on attachment 663637 [details] [diff] [review]
Part 1 - propagate errors correctly

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +1449,5 @@
>    JS::Value value = STRING_TO_JSVAL(string);
>    if (!JS_DefineProperty(args->cx, args->tags, aKey.Data(), value,
>                           NULL, NULL, JSPROP_ENUMERATE)) {
>      NS_WARNING("Failed to set metadata property");
> +    args->error = true;

Hmmmm.... Actually why should we fail here? Why not just continue and try to add the other metadata properties to the metadata object? It doesn't seem that users benefit from us throwing a JS exception here.

Comment 7

[Ralph Giles (:rillian)]

(In reply to Chris Pearce (:cpearce) from comment #6)

> Hmmmm.... Actually why should we fail here? Why not just continue and try to
> add the other metadata properties to the metadata object? It doesn't seem
> that users benefit from us throwing a JS exception here.

In bug 785057, Ms2ger and bz said a failure in JS_SetProperty put the context in a state where it should not be used again until the failure was propagated back to the interpreter. Maybe JS_DefineProperty is different, and maybe it's possible to clear the exception state on the context, but I didn't understand how to do that.

Comment 8

[Ralph Giles (:rillian)]

Attachment: Part 1 - propagate errors correctly

Update log message to correct the bug reference.

Comment 9

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

Comment on attachment 663857 [details] [diff] [review]
Part 1 - propagate errors correctly

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

Ok, let's run with this then.

Comment 10

[Ralph Giles (:rillian)]

Ok. Thanks for the review!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db94c9693123
https://hg.mozilla.org/integration/mozilla-inbound/rev/da60a3b8eeac

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/db94c9693123
https://hg.mozilla.org/mozilla-central/rev/da60a3b8eeac

Comment 13

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

(In reply to Ralph Giles (:rillian) from comment #7)
> (In reply to Chris Pearce (:cpearce) from comment #6)
> 
> > Hmmmm.... Actually why should we fail here? Why not just continue and try to
> > add the other metadata properties to the metadata object? It doesn't seem
> > that users benefit from us throwing a JS exception here.
> 
> In bug 785057, Ms2ger and bz said a failure in JS_SetProperty put the
> context in a state where it should not be used again until the failure was
> propagated back to the interpreter. Maybe JS_DefineProperty is different,
> and maybe it's possible to clear the exception state on the context, but I
> didn't understand how to do that.

No, this is true for all JSAPI functions. I would appreciate it that patches that involve the JSAPI get reviewed by someone who understands it...

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 663857 [details] [diff] [review]
Part 1 - propagate errors correctly

Does this look good to you?

Comment 15

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

Comment on attachment 663857 [details] [diff] [review]
Part 1 - propagate errors correctly

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

I didn't have any comments, or I would have made them :)

Comment 16

[Ralph Giles (:rillian)]

Ok! Thanks for the additional review. :-)