Closed Bug 793315 Opened 7 years ago Closed 7 years ago

Fix mozGetMetadata with single-tag streams

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: rillian, Assigned: rillian)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → giles
Attachment #663569 - Flags: review?(cpearce)
Well, that was embarrassing.
Attachment #663570 - Flags: review?(cpearce)
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.
Attachment #663569 - Flags: review?(cpearce)
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.
Attachment #663569 - Attachment is obsolete: true
Attachment #663637 - Flags: review?(cpearce)
Attachment #663570 - Flags: review?(cpearce) → review+
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.
(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.
Update log message to correct the bug reference.
Attachment #663637 - Attachment is obsolete: true
Attachment #663637 - Flags: review?(cpearce)
Attachment #663857 - Flags: review?(cpearce)
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.
Attachment #663857 - Flags: review?(cpearce) → review+
Ok. Thanks for the review!
Keywords: checkin-needed
Blocks: 455165
https://hg.mozilla.org/mozilla-central/rev/db94c9693123
https://hg.mozilla.org/mozilla-central/rev/da60a3b8eeac
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(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 on attachment 663857 [details] [diff] [review]
Part 1 - propagate errors correctly

Does this look good to you?
Attachment #663857 - Flags: feedback?(Ms2ger)
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 :)
Attachment #663857 - Flags: feedback?(Ms2ger)
Ok! Thanks for the additional review. :-)
You need to log in before you can comment on or make changes to this bug.