Fix mozGetMetadata with single-tag streams

RESOLVED FIXED in mozilla18

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

Trunk
mozilla18
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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)

Comment 1

6 years ago
Created attachment 663569 [details] [diff] [review]
Part 1 - propagate errors correctly
Assignee: nobody → giles
Attachment #663569 - Flags: review?(cpearce)
(Assignee)

Comment 2

6 years ago
Created attachment 663570 [details] [diff] [review]
Part 2 - verify the fix in the test suite

Well, that was embarrassing.
Attachment #663570 - Flags: review?(cpearce)
(Assignee)

Comment 4

6 years ago
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)
(Assignee)

Comment 5

6 years ago
Created attachment 663637 [details] [diff] [review]
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.
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.
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
Created attachment 663857 [details] [diff] [review]
Part 1 - propagate errors correctly

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
https://hg.mozilla.org/mozilla-central/rev/db94c9693123
https://hg.mozilla.org/mozilla-central/rev/da60a3b8eeac
Status: NEW → RESOLVED
Last Resolved: 6 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.