857765 - User defined PeerConnection onSuccess/onError callbacks throw away all caught errors

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Currently we catch all the exceptions which could be thrown in user-defined onSuccess/onError callbacks and throw them away. See:

http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js#771

773       try {
774         this._dompc._onCreateOfferSuccess.onCallback({
775           type: "offer", sdp: offer,
776           __exposedProps__: { type: "rw", sdp: "rw" }
777         });
778       } catch(e) {}

Similar to the gUM implementation those errors should probably end-up in window.onerror.

Anant, mentioned on IRC that it could be a probably easy fix. I will let him comment here.

This is a kinda blocker for our mochitests given that we miss important failures while executing the pc tests.

Comment 1

[Jason Smith [:jsmith]]

Definitely needs to block.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As Adam mentioned on IRC the fix for bug 842531 might fix this for real but it's not something we can wait for. Marking as dependency anyway to establish the connection between both.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: testcase

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Bobby or Blake, has one of you an idea? Would the rewrite in bug 842531 really be necessary to accomplish that?

Comment 5

[Bobby Holley (:bholley)]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Bobby or Blake, has one of you an idea? Would the rewrite in bug 842531
> really be necessary to accomplish that?

I don't have enough context here to comment.

Comment 6

[Boris Zbarsky [:bzbarsky]]

JS basically never has any sane control over exception reporting.  The only thing that can do it sanely, as far as I can tell, is C++.

Moving to a WebIDL implementation sitting in front of PeerConnection, even without fixing bug 842531 would almost certainly fix this.  Apart from that, your options are ... not many.  :(

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, getting our exception stuff right from C++ is already difficult. Getting it right directly from JS is even harder...

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Hi Ben, since you were so helpful on bug 834933 (throw Components.Exception), I was hoping you knew what was needed here. :-)

> http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js#771
> 
> 773       try {
> 774         this._dompc._onCreateOfferSuccess.onCallback({
> 775           type: "offer", sdp: offer,
> 776           __exposedProps__: { type: "rw", sdp: "rw" }
> 777         });
> 778       } catch(e) {}

So the context here is we'd like exceptions in user-provided callbacks to get caught and shown in Web Console, just like they do in gUM.

Now, gUM is implemented in C++, so what we're observing there is that by the time the JS callback comes back to C++, there is no error as far as the C++ code knows, and the error just appears in Web Console (immediately or later, I forget). In other words, it looks like XPConnect catches any JS exception before the thread re-enters C++ and "just handles it".

The code above in PeerConnection.js, on the other hand, is not C++ obviously, yet is also behind XPConnect. It appears to have no such luck. Anant said on IRC that he recalled originally adding the try{}catch(e){} you see here because it crashed otherwise (I'll retest that next).

I was wondering if you knew the right way to handle this.

Comment 9

[Boris Zbarsky [:bzbarsky]]

So one thing you could try doing is manually reporting to the web console in the catch clause there...

XPConnect purposefully does not report exceptions if there's JS on the stack, for what it's worth.

Comment 10

[Anant Narayanan [:anant]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)
> The code above in PeerConnection.js, on the other hand, is not C++
> obviously, yet is also behind XPConnect. It appears to have no such luck.
> Anant said on IRC that he recalled originally adding the try{}catch(e){} you
> see here because it crashed otherwise (I'll retest that next).

To be clear, the try catch is necessary because an exception in the user provided callback would halt the execution of PeerConnection.js.

As a short term fix, you could use https://developer.mozilla.org/en-US/docs/Components.utils.reportError to log to the console as bz mentions, though the ideal fix would be to invoke window.onerror. If there isn't a way to do that from JS, would it be worth adding such a mechanism?

Comment 11

[Adam Roach [:abr]]

(In reply to Anant Narayanan [:anant] from comment #10)
> 
> To be clear, the try catch is necessary because an exception in the user
> provided callback would halt the execution of PeerConnection.js.
> 
> As a short term fix, you could use
> https://developer.mozilla.org/en-US/docs/Components.utils.reportError to log
> to the console as bz mentions, though the ideal fix would be to invoke
> window.onerror. If there isn't a way to do that from JS, would it be worth
> adding such a mechanism?

Keep in mind that this is short-term only. Long-term (Firefox 23 or 24), these need to be genuine events. I presume those will be handled "the right way", as events are an integral part of proper JS behavior.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> would it be worth adding such a mechanism?

That's very hard, actually, because the exception object does not have the information needed to properly invoke window.onerror, last I checked.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I've done some tests, and looking in the Error Console (rather than the Web Console) shows that an error *is* thrown:

  Timestamp: 4/8/13 1:55:09 PM
  Error: [Exception... "'Error: Error thrown in onCreateOfferSuccess callback.'
   when calling method: [RTCPeerConnectionCallback::onCallback]"
    nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"
    location: "JS frame :: file:///.../components/PeerConnection.js
   :: PeerConnectionObserver.prototype.onCreateOfferSuccess :: line 776"
    data: no]
  Source File: file:///.../components/PeerConnection.js
  Line: 776

The question is why it doesn't make it to the web console. Interestingly, running the same test on the onCreateOfferFailure (rather than Success) callback fares the same plus a warning from webconsole.js:

  Timestamp: 4/8/13 1:45:05 PM
  Error: [Exception... "'Error: Error thrown in onCreateOfferFailure callback.'
   when calling method: [RTCPeerConnectionCallback::onCallback]"
    nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"
    location: "JS frame :: file:///.../components/PeerConnection.js
   :: PeerConnectionObserver.prototype.callErrorCallback :: line 765"
    data: no]
  Source File: file:///.../components/PeerConnection.js
  Line: 765

  Timestamp: 4/8/13 1:45:06 PM
  Warning: ReferenceError: reference to undefined property aItem.type
  Source File: chrome://browser/content/devtools/webconsole.js
  Line: 2272

A clue?

About PeerConnection.js: If I remove the try{}catch(e){} mentioned then nothing appears to crash, but errors appear twice in the Error Console. Looks like Vanellope cannot leave candy-land.

Comment 14

[Adam Roach [:abr]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #13)

> About PeerConnection.js: If I remove the try{}catch(e){} mentioned then
> nothing appears to crash, but errors appear twice in the Error Console.
> Looks like Vanellope cannot leave candy-land.

Be careful here. If you don't catch the exception, the function won't finish running (meaning it won't grab the next action in the queue and run it). You can end up with operations in the queue being stranded.

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Log errors thrown by user-defined PeerConnection callbacks.

Work in progress. Outputs to web console, like this:

 'Error: Hi!' when calling method [RTCPeerConnectionCallback::onCallback]
 PeerConnection.js:781

It has a number of limitations:
- Does not show which callback it was (I hope to address this),
- Shows pc.js file and line no. rather than for user's throw (no can do),
- Shows as gray "Logging" msg, not orange "JS" msg in Web Console.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Log errors thrown by user-defined PeerConnection callbacks. (2)

Output now indicates which callback threw. e.g.

 'Error: Hi!' when calling method [RTCPCCallback::onCreateOfferError]
 PeerConnection.js:781

Some refactoring.

Comment 17

[Jason Smith [:jsmith]]

This really needs a test. Even if it's just something dirt simple that verifies that an unchecked exception within a success and an error callback results it coming up the callstack and failing.

Have we proven that our automation will fail on exceptions with success and error callbacks?

Comment 18

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I think a test is a good idea.

But success/failure callbacks are the end of the road for the discrete methods that use them, so exceptions thrown in them don't travel up any callstack, as there's nothing more to halt inside the methods they belong to. Similarly, the "onChange" callbacks, don't have the power to terminate anything, hence the try/catch around every callback. 

A webrtc test pushes its own ball forward, like tennis, so a progress-fail test would only be testing its own logic, i.e. ability to hit balls, when it should be testing the other player (testing PeerConnection's ability to tear itself down in any state seems useful but unrelated to this code change).

I suggest I write a test that throws in each callback (and continues, for expediency). That should exercise the new code and catch regressions. Is there a way to capture and compare output from web console?

Comment 19

[Jason Smith [:jsmith]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #18)
> I suggest I write a test that throws in each callback (and continues, for
> expediency). That should exercise the new code and catch regressions. Is
> there a way to capture and compare output from web console?

Don't know. Hmm...Henrik do you know?

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Log errors thrown by user-defined PeerConnection callbacks. (3)

Unbitrotted.

A mochitest is going to take a little more work. I'll have to orchestrate failure cases to exercise all failure callbacks. Is this something we have already?

Attaching this so Adam can review code in the meantime.

Comment 21

[Jason Smith [:jsmith]]

Targeted to ship for Fx22, tracking to get on rel mgmt radar.

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> Is there a way to capture and compare output from web console?

Stepping back, Ideally, a webdev calling peerconnection.createOffer(myoffer, success, failure), where success() throws new Error("hi"), would see an "orange" JS error in Web Console, just like for any other uncaught exception returning to a browser builtin. Then a mochitest would just override window.onerror to test it.

Problem is, while a real error does appear in the *error* console, nothing appears in web console.

Hence where we are: logging to console.error instead, creating "gray" error messages, and worse: contemplating logging to insecure _win.wrappedJSObject.console.error instead so a mochitest can override console to test the error.

If anyone has a better idea, please let me know.

Comment 23

[Mihai Sucan [:msucan]]

Instead of using console.error() create an nsIScriptError with values coming from the caught exception. This can be done from JavaScript (nsIScriptError is scriptable).

Make sure you set the window ID to be the ID of the window where the error happens.

Comment 24

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Log errors thrown by user-defined PeerConnection callbacks. (4)

Updated. Ready for review.
- Logs ScriptError to ConsoleService.
- Calls this._dompc._win.onerror() directly if present in DOM. Secure?
- Mochitest.

Filed separate XPConnect bug 862153 on filename+line-numbers.

Comment 25

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

A (mostly) green try: https://tbpl.mozilla.org/?tree=Try&rev=b039cc87194e

A perma-orange from just-added mochi on Android:

9401 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_bug857765.html |
Unexpected error callback from http://mochi.test:8888/tests/dom/media/tests/mochitest/head.js:135 with
name = 'ReferenceError', message = 'mozRTCPeerConnection is not defined'

I see the other mochi's use runTest(..., desktopSupportedOnly). Will fix. Are we still not testing there?

Comment 26

[Jason Smith [:jsmith]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #25)
> A (mostly) green try: https://tbpl.mozilla.org/?tree=Try&rev=b039cc87194e
> 
> A perma-orange from just-added mochi on Android:
> 
> 9401 ERROR TEST-UNEXPECTED-FAIL |
> /tests/dom/media/tests/mochitest/test_peerConnection_bug857765.html |
> Unexpected error callback from
> http://mochi.test:8888/tests/dom/media/tests/mochitest/head.js:135 with
> name = 'ReferenceError', message = 'mozRTCPeerConnection is not defined'
> 
> I see the other mochi's use runTest(..., desktopSupportedOnly). Will fix.
> Are we still not testing there?

Not yet. I'm planning on working on getting these tests working on Android as soon as gcp lands the build patch to start building WebRTC by default on Android.

Comment 27

[Jason Smith [:jsmith]]

Comment on attachment 737778 [details] [diff] [review]
Log errors thrown by user-defined PeerConnection callbacks. (4)

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

review- for potential reliability issues on the mochitest for using setTimeout.

::: dom/media/tests/mochitest/Makefile.in
@@ +38,5 @@
>    test_peerConnection_bug827843.html \
>    test_peerConnection_bug834153.html \
>    test_peerConnection_bug835370.html \
>    test_peerConnection_bug840344.html \
> +  test_peerConnection_bug857765.html \

Small nit - Can we switch to using descriptive naming for this test? For maintenance, it's a bit of a pain having to open up each file named "bugX" to figure out what it does.

::: dom/media/tests/mochitest/test_peerConnection_bug857765.html
@@ +43,5 @@
> +            pc2.setRemoteDescription(offer, function() {
> +              pc2.createAnswer(function(answer) {
> +                pc2.setLocalDescription(answer, function() {
> +                  pc1.setRemoteDescription(answer, function() {
> +                    setTimeout(finish, 10);

This isn't going to be reliable - we can't rely on setTimeout in tests typically, as that will lead to intermittent failures in CI.

@@ +63,5 @@
> +  });
> +
> +  function finish() {
> +    window.onerror = oldOnError;
> +    ok(error_count == 7, "Seven expected errors verified.");

The concept of this test looks okay, but the flow needs work to improve reliability.

What I think you should be doing instead that will remove the setTimeout usage is that in the window.onerror callback, you should check if you've hit your target error count you are waiting for. If you hit it, call SimpleTest.finish().

Comment 28

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Sorry, the n-i request got buried in my email somehow. It looks like you have a plan here though, right?

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

No worries, I think this should be good, but feel free to look it over.

Comment 30

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> Can we switch to using descriptive naming for this test?

Good idea. How about test_peerConnection_throwInCallbacks.html?

> What I think you should be doing instead that will remove the setTimeout usage
> is that in the window.onerror callback, you should check if you've hit your
> target error count you are waiting for. If you hit it, call SimpleTest.finish().

Good point. I'll do it that way.

Comment 31

[Jason Smith [:jsmith]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #30)
> > Can we switch to using descriptive naming for this test?
> 
> Good idea. How about test_peerConnection_throwInCallbacks.html?

That works.

Comment 32

[Mihai Sucan [:msucan]]

Comment on attachment 737778 [details] [diff] [review]
Log errors thrown by user-defined PeerConnection callbacks. (4)

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

Giving r+ for the nsIScriptError stuff. Still, it's rather ugly to have to parse the stackframe list like that, but I see why you do that (thanks for explaining it on IRC). As discussed on IRC please try to add a clearer explanation for the try-catch, about the issues it works around.

I also agree with Jason about the setTimeout() - this can easily cause oranges on tbpl. We need to avoid those as much as possible.

::: dom/media/PeerConnection.js
@@ +796,5 @@
> +            getService(Ci.nsIConsoleService).logMessage(scriptError);
> +
> +        // Call onerror directly if present (necessary for testing)
> +        if (typeof this._dompc._win.onerror === "function") {
> +          this._dompc._win.onerror(msg, file, line);

What happens if the content author uses an 'error' event listener? Something like document.addEventListener('error', foo).

Is it possible to synthesize an error event using the DOM event APIs?

::: dom/media/tests/mochitest/test_peerConnection_bug857765.html
@@ +1,3 @@
> +<!DOCTYPE HTML>
> +<html>
> +<head>

Make sure you add a <meta charset="utf8"> here, to avoid warnings.

Comment 33

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Log errors thrown by user-defined PeerConnection callbacks. (5) r=msucan

Update based on Jason & Mihas's feedback. Carrying forward r+ from Mihas.
- Mochi w/non-setTimout + desktop-only + rename.
- C++ code comments.

> What happens if the content author uses an 'error' event listener?
> Something like document.addEventListener('error', foo).
>
> Is it possible to synthesize an error event using the DOM event APIs?

Don't know the answer. The primary driver behind getting window.onerror to work here was that the mochitest required it, so I'd be happy to address that in a followup bug.

Comment 34

[Jason Smith [:jsmith]]

Comment on attachment 738073 [details] [diff] [review]
Log errors thrown by user-defined PeerConnection callbacks. (5) r=msucan

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

review+ on test. Some small nits in comments.

::: dom/media/tests/mochitest/Makefile.in
@@ +38,5 @@
>    test_peerConnection_bug827843.html \
>    test_peerConnection_bug834153.html \
>    test_peerConnection_bug835370.html \
>    test_peerConnection_bug840344.html \
> +  test_peerConnection_throwInCallbacks.html \

Nit - I'd put this under test_peerConnection_offerRequiresReceiveVideoAudio.html

::: dom/media/tests/mochitest/test_peerConnection_throwInCallbacks.html
@@ +20,5 @@
> +    error_count += 1;
> +    info("onerror " + error_count + ": " + errorMsg);
> +    if (error_count == 7) {
> +      finish();
> +    }

Not sure if this matters, but should we add an else if clause here:

else if(error_count > 7) {
   ok(false, "Unexpected error count - " + error_count);
}

@@ +63,5 @@
> +  }, 1);
> +
> +  function finish() {
> +    window.onerror = oldOnError;
> +    ok(error_count == 7, "Seven expected errors verified.");

Technically if you've entered this function, you've already verified that the error count is 7. So this is probably not needed.

Comment 35

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> Nit - I'd put this under test_peerConnection_offerRequiresReceiveVideoAudio.html

Sure np.

> Not sure if this matters, but should we add an else if clause here:

No that's a good point. Willdo.

> Technically if you've entered this function, you've already verified that the
> error count is 7. So this is probably not needed.

I agree it is redundant, but the test framework yells at me if I don't have at least one ok() or is(), and ok(true, "...") seemed disingenuous, so that's why it's there.

Comment 36

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> else if(error_count > 7) {
>   ok(false, "Unexpected error count - " + error_count);
> }

Sorry, I confused myself there. Looking at it again: 1 and the test ends at 7 I don't see how it can ever become 8, so I don't think we need to test for that.

But what I think would be good (and what I may have been thinking when you brought up the else clause) is to add a test for errors without the word "Expected" in them and fail in that case.

Comment 37

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

s/1/Since the error count increments by 1/

Comment 38

[Adam Roach [:abr]]

Comment on attachment 738073 [details] [diff] [review]
Log errors thrown by user-defined PeerConnection callbacks. (5) r=msucan

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

Looks fine. The only thing that really needs to be fixed is the invocation of unexpectedCallbackAndFinish.

::: dom/media/PeerConnection.js
@@ +812,5 @@
> +        // Call onerror directly if present (necessary for testing)
> +        if (typeof this._dompc._win.onerror === "function") {
> +          this._dompc._win.onerror(msg, file, line);
> +        }
> +      }

I didn't really understand all of that, but I assume that this is the part that Mihai reviewed.

@@ +819,5 @@
>  
>    onCreateOfferSuccess: function(offer) {
> +    this.callCB(this._dompc._onCreateOfferSuccess,
> +                { type: "offer", sdp: offer,
> +                __exposedProps__: { type: "rw", sdp: "rw" } });

This feels half-done. You make an RTCError class but not an RTCSessionDescription class. But that's just a nit. I wouldn't block landing this bug on adding new class; it just seems like something that would be easy to do here.

::: dom/media/tests/mochitest/test_peerConnection_throwInCallbacks.html
@@ +69,5 @@
> +  }
> +
> +  function wrong(err) {
> +    window.onerror = oldOnError;
> +    unexpectedCallbackAndFinish(err)

With the landing of Bug 860952, this signature has changed. To make it minimally correct, it needs to be something like:

unexpectedCallbackAndFinish(new Error)(err);

Although this will defeat the purpose of Bug 860952, since it will report line 73 for all errors. What you really want is to change "function wrong" to be something like this:

function wrong(where) {
  return function (err) {
    window.onerror = onOldError;
    unexpectedCallbackAndFinish(where)(err);
  };
}

And your failure callbacks should be "wrong(new Error)" rather than simply "wrong".

Comment 39

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

>> +          this._dompc._win.onerror(msg, file, line);
>> +        }
>> +      }
>
> I didn't really understand all of that, but I assume that this is the part
> that Mihai reviewed.

I think so. I hope calling window.onerror like this is safe though.

> This feels half-done. You make an RTCError class but not an
> RTCSessionDescription class.

Guilty. I factored out RTCError solely to simplify the callback. We can add the rest later I think.

> unexpectedCallbackAndFinish(new Error)(err);

Ick. Excessively clever. Thanks for the heads-up.

> function wrong(where) {
>   return function (err) {
>     window.onerror = onOldError;
>     unexpectedCallbackAndFinish(where)(err);
>   };
> }

Careful. This would actually do the wrong thing: It would reset window.onerror immediately on the first line that tried to "use" wrong. Wrong is now called to *set up* the error callback, rather than when/if the callback is ever called (the callback itself is now inaccessible).

I see a lot of bugs in this API's future. Maybe worth mentioning on the list as a downside to the double-callback API?

I may be able to factor out the wrong function entirely, if I don't care about leaving window.onerror set... Hmm.

Comment 40

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Nevermind, I read the code wrong. Looks good. Still not a fan fwiw, as my poor little c++ brain blew a circuit reading that code. :-)

Comment 41

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I renamed it for my sanity:

  function getFail(where) {
    return function (err) {
      window.onerror = onOldError;
      unexpectedCallbackAndFinish(where)(err);
    };
  }

Thanks for that code and the review!

Comment 42

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Log errors thrown by user-defined PeerConnection callbacks. (6) r=msucan, abr, jsmith

Updated with nits and un-bitrotted. Carrying forward r+ from Mihai, Jason & Adam.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Just to make sure, we're not munging the window.onerror on the content document, right?

Comment 44

[Randell Jesup [:jesup] (needinfo me)]

Removing checkin-needed until bz's question has been answered

Comment 45

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> Just to make sure, we're not munging the window.onerror on the content document,
> right?

Munging as in overwriting? Only in the mochitest, and it restores the original afterwards.

In the code itself, PeerConnection checks if window.onerror on the content document is set and calls it. That shouldn't munge it should it?

Comment 46

[Boris Zbarsky [:bzbarsky]]

No, that should be fine.

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 738257 [details] [diff] [review]
Log errors thrown by user-defined PeerConnection callbacks. (6) r=msucan, abr, jsmith

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

Jan, thanks a lot for this fix! It will certainly help us a lot. Have we run it through try first to ensure we don't unhide hidden failures?

::: dom/media/tests/mochitest/test_peerConnection_throwInCallbacks.html
@@ +66,5 @@
> +  }, 1);
> +
> +  function finish() {
> +    window.onerror = oldOnError;
> +    ok(error_count == 7, "Seven expected errors verified.");

A nit but good to do in the future, this should have been is(error_count, 7, ...).

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55974ab3d9af

Comment 49

[Mihai Sucan [:msucan]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #39)
> >> +          this._dompc._win.onerror(msg, file, line);
> >> +        }
> >> +      }
> >
> > I didn't really understand all of that, but I assume that this is the part
> > that Mihai reviewed.
> 
> I think so. I hope calling window.onerror like this is safe though.

I'm not a security expert and I am not sure what kind of security requirements we have here, however do note that calling window.onerror() obviously means you will execute content code. That code can throw and can do anything the page author wants. Afaik, it cannot gain chrome privileges, so that should be fine.

Comment 50

[Adam Roach [:abr]]

(In reply to Mihai Sucan [:msucan] from comment #49)

> I'm not a security expert and I am not sure what kind of security
> requirements we have here, however do note that calling window.onerror()
> obviously means you will execute content code. That code can throw and can
> do anything the page author wants. Afaik, it cannot gain chrome privileges,
> so that should be fine.

Ooh, good point. If the content code throws and we don't catch it, we'll stop executing here. That means the PeerConnection event pump might stop. We really need to wrap that call in a try/catch block. I think a follow-on patch is warranted. Either that or we can open a new big to fix it. Jan-Ivar? Your preference?

Comment 51

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Good point. I'll upload a follow-on patch here right away.

Comment 52

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> Have we run it through try first to ensure we don't unhide hidden failures?

Yes, there's one in Comment 25 (green except for missing DesktopOnly at the time), and there's only been comments and mochi timing-changes since then.

I did not intend to interfere with reporting of other errors, though I see I made an assumption that there'd be none (unknown errors fail the new test). Seems to be ok... How do we feel about it?

> A nit but good to do in the future, this should have been is(error_count, 7, ...).

I'll fix this as well in the follow-on patch.

Comment 53

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Follow-on patch: Adds try/catch to content-script window.onerror call.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55974ab3d9af

Comment 55

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Follow-on patch: Adds try/catch to content-script window.onerror call. (2)

Updated to log to console any exception window.onerror may throw.

Comment 56

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I checked with bz on irc and we should be ok with calling windows.onerror. File and line number from any exception it may throw may be bogus but ok to read.

Comment 57

[Adam Roach [:abr]]

Comment on attachment 738778 [details] [diff] [review]
Follow-on patch: Adds try/catch to content-script window.onerror call. (2)

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

Looks good.

Comment 58

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

First patch already landed. Only follow-on patch needs check-in.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dd64ef21eff

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9dd64ef21eff

Comment 61

[Jason Smith [:jsmith]]

Verified via mochitest on checkin.

Comment 62

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 738257 [details] [diff] [review]
Log errors thrown by user-defined PeerConnection callbacks. (6) r=msucan, abr, jsmith

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A

User impact if declined: Developers won't get any logs of errors that occur in their callbacks, making it harder/confusing for them to debug their applications.  This fix has already helped Henrik track down a bug in his datachannels mochitest code.

Testing completed (on m-c, etc.): on m-c, verified by QA

Risk to taking this patch (and alternatives if risky): Moderately low; tocuhes/cleans-up a bunch of try/catch blocks; calls window onerror directly.   Alternative is to tell developers to either debug in nightly or include some internal catches that use dump() or some such.

String or IDL/UUID changes made by this patch: none

Comment 63

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 738778 [details] [diff] [review]
Follow-on patch: Adds try/catch to content-script window.onerror call. (2)

[Approval Request Comment]
See other request in this bug

Comment 64

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4f1a300cb8e7
https://hg.mozilla.org/releases/mozilla-aurora/rev/0908136c0301