785057 - Assertion failure: !cx->isExceptionPending() because nsHTMLMediaElement::BuildObjectFromTags doesn't handle failure

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

When a JSAPI function throws an exception, you need to stop using that context... See $URL.

Comment 1

[Boris Zbarsky [:bzbarsky]]

In particular, this part is buggy:

1445   if (!JS_SetProperty(args->cx, args->tags, aKey.Data(), &value)) {
1446     NS_WARNING("Failed to set metadata property");
1447   }

That case needs to stop the iteration instead.  Not only that, but it needs to ensure that an error rv is returned from MozGetMetadata.  Alternately, if failure here should not throw to JS, it should explicitly clear the exception on cx.

That said, it's a bit odd that this stuff is being affected by Object.prototype at all.  Shouldn't this code be using JS_DefineProperty instead of JS_SetProperty?  If it did that, we'd definitely want JS_DefineProperty failure to throw.

Comment 2

[Ralph Giles (:rillian)]

Thanks for the pointer. I used JS_SetProperty because I didn't need the finer control JS_DefineProperty offers. To quote the JSAPI Reference, '"Define" is a lower-level version of "set" that provides access to extra settings and does not call setters.' That sounded like what I wanted.

I'll change the code to return an error from MozGetMetadata.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, the question is whether you want to call setters off Object.prototype or not...  Seems a bit weird to, honestly.

Comment 4

[Ralph Giles (:rillian)]

Attachment: proposed fix

My initial reaction here was 'dynamic language is dynamic' but I've been convinced calling DefineProperty is less surprising here. The attached patch should address both issues.

Thanks to Boris and Graydon for the explanations.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 660944 [details] [diff] [review]
proposed fix

r=me

Comment 6

[Ralph Giles (:rillian)]

Thanks!

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

Comment 7

[Ralph Giles (:rillian)]

Try is green.

Comment 8

[Ralph Giles (:rillian)]

No testsuite to verify the exception is fixed; the URL link is sufficient for that.

Positive functionality of the code is verified by existing tests.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ralph Giles (:rillian) from comment #6)
> Pushed to try as https://tbpl.mozilla.org/?tree=Try&rev=0a57eabc8f65

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7689bfe3675d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7689bfe3675d

Comment 11

[Ralph Giles (:rillian)]

Ms2ger: can you be more specific about what you're after here?

Comment 12

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

The usual: a test that failed before the code change, and that passes now.

Comment 13

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

Bug 793315 shows why testing this code would have been useful.

Comment 14

[Ralph Giles (:rillian)]

Ok, thanks.

Given all the complaints about cycle times I don't feel it's worth writing a test for the getter override. The script in the URL link is sufficient for qa to verify the bug is resolved.

Comment 15

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

This isn't about QA verifying anything, but about automated regression testing. Not sure what your comment about cycle times is about.