836625 - about:net-internals can leave random pending exceptions lying around

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The code checked in in bug looks to me like it can leave exceptions lying around on the safe JS context when it returns to the event loop.  For example, Dashboard::GetSockets runs directly off the event loop and as far as I can tell, and has all sorts of returns on things that set pending exceptions without ever reporting or clearing them.  Again, as far as I can tell.

What I don't quite understand, honestly, is why we're doing all this manual jsapi stuff.  We have various binding code that can do these sorts of things, and if it's lacking somehow for this use case we can fix it as needed instead of hand-rolling error-prone JSAPI code...

Comment 1

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #0)
> The code checked in in bug looks...

bug 783205

Comment 2

[Peter Van der Beken [:peterv]]

Also, you really shouldn't use nsContentUtils from netwerk/ if you can avoid it. You can get the context stack from XPConnect directly (through @mozilla.org/js/xpc/ContextStack;1).

Comment 3

[Valentin Gosu [:valentin] (he/him)]

 (In reply to Boris Zbarsky (:bz) from comment #0)
> What I don't quite understand, honestly, is why we're doing all this manual
> jsapi stuff.  We have various binding code that can do these sorts of
> things, and if it's lacking somehow for this use case we can fix it as
> needed instead of hand-rolling error-prone JSAPI code...

@bz
At the time it seemed like the best way to achieve this.
Could you provide a link to the helper code I could use instead of this approach?

Comment 4

[Boris Zbarsky [:bzbarsky]]

What I think we should do is see if these objects can be expressed as WebIDL dictionaries.  If so, I would recomend doing that and using existing facilities for converting a dictionary to a JS value.  See <https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Dictionary_types>.  There would still need to be manual exception-reporting, unfortunately, but it would happen at a single chokepoint and you wouldn't need the rest of the JSAPI usage.

Note that the dictionary bits involved have been in the tree since October.  If they're not sufficient for some reason and you're interested in using WebIDL here, please let me know what's missing and I'll fix it.

But obviously this bug can be fixed while sticking to manual JSAPI too.  If we want to do the dictionary thing it might want a separate bug.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> 
> Note that the dictionary bits involved have been in the tree since October. 
> If they're not sufficient for some reason and you're interested in using
> WebIDL here, please let me know what's missing and I'll fix it.
> 

I'm trying to convert it all to webidl, however, I'm running into the next issue:

interface NetDashboard {
readonly attribute DOMString[] host;
...
};

The error I get is:

  File "/home/icecold/ff_trunk_debug1/dom/bindings/Codegen.py", line 3612, in getRetvalDeclarationForType
    returnType)
TypeError: Don't know how to declare return value for StringArray

I also tried defining the attribute as a sequence<DOMString>, but apparently that's against the webidl specifications and so it doesn't work.

Comment 6

[Boris Zbarsky [:bzbarsky]]

What do you want this .host attribute to actually return?  If it's a new JS array every time, you should probably use a method, not an attribute, right?

That said, the current API has no such methods. It just hands out JS objects (in WebIDL those would be dictionaries) with a "host" array on them (a sequence<DOMString> member in the dictionary in WebIDL terms).