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

VERIFIED FIXED in Firefox 22

Status

()

P2
major
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: whimboo, Assigned: jib)

Tracking

({testcase})

18 Branch
mozilla23
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox21 disabled, firefox22+ fixed, firefox23 fixed)

Details

(Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked])

Attachments

(3 attachments, 6 obsolete attachments)

556 bytes, text/html
Details
16.47 KB, patch
jib
: review+
Details | Diff | Splinter Review
3.83 KB, patch
abr
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
Definitely needs to block.
Whiteboard: [WebRTC][blocking-webrtc?][automation-blocked] → [WebRTC][blocking-webrtc+][automation-blocked]
(Reporter)

Comment 2

5 years ago
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.
Depends on: 842531
(Reporter)

Comment 3

5 years ago
Created attachment 733046 [details]
testcase
(Reporter)

Updated

5 years ago
Keywords: testcase
(Reporter)

Comment 4

5 years ago
Bobby or Blake, has one of you an idea? Would the rewrite in bug 842531 really be necessary to accomplish that?
Flags: needinfo?(mrbkap)
Flags: needinfo?(bobbyholley+bmo)
(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.
Flags: needinfo?(bobbyholley+bmo)
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.  :(
(Reporter)

Updated

5 years ago
Whiteboard: [WebRTC][blocking-webrtc+][automation-blocked] → [WebRTC][blocking-webrtc+]
(Reporter)

Updated

5 years ago
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][qa-automation-blocked]

Updated

5 years ago
Assignee: nobody → jib
Yeah, getting our exception stuff right from C++ is already difficult. Getting it right directly from JS is even harder...
Flags: needinfo?(mrbkap)
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.
Flags: needinfo?(bent.mozilla)
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.
(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

5 years ago
(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.
> 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.
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

5 years ago
(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.
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked] → [WebRTC][blocking-webrtc+][qa-automation-blocked][webrtc-uplift]
Created attachment 735520 [details] [diff] [review]
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.
Created attachment 736061 [details] [diff] [review]
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.
Attachment #735520 - Attachment is obsolete: true
Attachment #736061 - Flags: review?(adam)
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?
Flags: in-testsuite?
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?
(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?
Flags: needinfo?(hskupin)
Created attachment 736324 [details] [diff] [review]
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.
Attachment #736061 - Attachment is obsolete: true
Attachment #736061 - Flags: review?(adam)
Attachment #736324 - Flags: review?(adam)
Targeted to ship for Fx22, tracking to get on rel mgmt radar.
tracking-firefox22: --- → ?
> 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.
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.

Updated

5 years ago
Flags: needinfo?(hskupin)

Updated

5 years ago
status-firefox22: --- → affected
tracking-firefox22: ? → +
Created attachment 737778 [details] [diff] [review]
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.
Attachment #736324 - Attachment is obsolete: true
Attachment #736324 - Flags: review?(adam)
Attachment #737778 - Flags: review?(mihai.sucan)
Attachment #737778 - Flags: review?(adam)
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?
(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 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().
Attachment #737778 - Flags: review-
Sorry, the n-i request got buried in my email somehow. It looks like you have a plan here though, right?
Flags: needinfo?(bent.mozilla)
No worries, I think this should be good, but feel free to look it over.
> 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.
(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 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.
Attachment #737778 - Flags: review?(mihai.sucan) → review+
Created attachment 738073 [details] [diff] [review]
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.
Attachment #737778 - Attachment is obsolete: true
Attachment #737778 - Flags: review?(adam)
Attachment #738073 - Flags: review?(jsmith)
Attachment #738073 - Flags: review?(adam)
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.
Attachment #738073 - Flags: review?(jsmith) → review+
> 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.
> 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.
s/1/Since the error count increments by 1/

Comment 38

5 years ago
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".
Attachment #738073 - Flags: review?(adam) → review+
>> +          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.
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. :-)
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!
Created attachment 738257 [details] [diff] [review]
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.
Attachment #738073 - Attachment is obsolete: true
Attachment #738257 - Flags: review+
Keywords: checkin-needed
Just to make sure, we're not munging the window.onerror on the content document, right?
Removing checkin-needed until bz's question has been answered
Flags: needinfo?(jib)
Keywords: checkin-needed
> 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?
Flags: needinfo?(jib)
No, that should be fine.
Keywords: checkin-needed
(Reporter)

Comment 47

5 years ago
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, ...).
https://hg.mozilla.org/integration/mozilla-inbound/rev/55974ab3d9af
status-firefox21: --- → disabled
status-firefox23: --- → affected
Keywords: checkin-needed
Target Milestone: --- → mozilla23
(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

5 years ago
(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?
Flags: needinfo?(jib)
Good point. I'll upload a follow-on patch here right away.
Flags: needinfo?(jib)

Updated

5 years ago
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked][webrtc-uplift] → [leave-open][WebRTC][blocking-webrtc+][qa-automation-blocked][webrtc-uplift]
> 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.
Created attachment 738552 [details] [diff] [review]
Follow-on patch: Adds try/catch to content-script window.onerror call.
Attachment #738552 - Flags: review?(adam)
Created attachment 738778 [details] [diff] [review]
Follow-on patch: Adds try/catch to content-script window.onerror call. (2)

Updated to log to console any exception window.onerror may throw.
Attachment #738552 - Attachment is obsolete: true
Attachment #738552 - Flags: review?(adam)
Attachment #738778 - Flags: review?(adam)
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

5 years ago
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.
Attachment #738778 - Flags: review?(adam) → review+
First patch already landed. Only follow-on patch needs check-in.
Keywords: checkin-needed
Whiteboard: [leave-open][WebRTC][blocking-webrtc+][qa-automation-blocked][webrtc-uplift] → [WebRTC][blocking-webrtc+][qa-automation-blocked][webrtc-uplift]
https://hg.mozilla.org/mozilla-central/rev/9dd64ef21eff
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Verified via mochitest on checkin.
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+

Updated

5 years ago
status-firefox23: affected → fixed
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
Attachment #738257 - Flags: approval-mozilla-aurora?
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
Attachment #738778 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #738257 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #738778 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/4f1a300cb8e7
https://hg.mozilla.org/releases/mozilla-aurora/rev/0908136c0301
status-firefox22: affected → fixed
Whiteboard: [WebRTC][blocking-webrtc+][qa-automation-blocked][webrtc-uplift] → [WebRTC][blocking-webrtc+][qa-automation-blocked]
You need to log in before you can comment on or make changes to this bug.