1000947 - Console::Method can throw, but it's not annotated as [Throws] in WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

When running debug builds, I'm getting a ton of assertions here:
http://mxr.mozilla.org/mozilla-central/source/js/src/jscntxtinlines.h#242

The problem seems to be that Console::Method can throw exceptions:
http://mxr.mozilla.org/mozilla-central/source/dom/base/Console.cpp#860

However, the methods like log, info, and warn are not annotated as [Throws] in the WebIDL file. I think we need to add the annotation and change the return type of Console::Method so that we can return false in these cases.

Comment 1

[Andrea Marchesini [:baku]]

> However, the methods like log, info, and warn are not annotated as [Throws]
> in the WebIDL file. I think we need to add the annotation and change the
> return type of Console::Method so that we can return false in these cases.

I don't think we should throw exceptions in the console API.
Unfortunately we don't have any spec for this API yet, but I think that the console API is meant to be for debugging, and no exceptions should be thrown when the developer is debugging his code.

Do you have any testcase when this issue happens?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Oho!  This could be what dbaron was seeing in bug 990789.

If you don't want this code to throw, then you should remove the Throw calls and add a ClearException on the stack or something.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Requesting tracking, since this can cause script running after the failing console call to randomly fail.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I haven't been able to reproduce this reliably. It's probably the same issue as David was seeing, since I also saw it on nytimes.com.

By the way, Boris, it might help to instrument the generated code to check for exceptions thrown from code that's not supposed to throw. Otherwise we don't catch it until CallJSNative, at which time it's harder to figure out from the crash which function is at fault. I'm not sure how common this is in general, though.

Comment 5

[(no longer active)]

(In reply to comment #4)
> By the way, Boris, it might help to instrument the generated code to check for
> exceptions thrown from code that's not supposed to throw. Otherwise we don't
> catch it until CallJSNative, at which time it's harder to figure out from the
> crash which function is at fault. I'm not sure how common this is in general,
> though.

I'd say we should MOZ_ASSERT that!

Comment 6

[Boris Zbarsky [:bzbarsky]]

Indeed.  Filed bug 1001157.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: throw.patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8412479 [details] [diff] [review]
throw.patch

The string bundle service is not threadsafe.  So you can't use it on the worker thread.

I'm not entirely sure that reporting a generic message like this is terribly useful anyway, honestly...  Esp. since the generic string is not even right for some of the callers (e.g. profile()).

If we do want to report something, we should do it on mainthread, put the localization bits in dom.properties, and use the nsContentUtils helpers we have for this purpose.

Comment 9

[Andrea Marchesini [:baku]]

> I'm not entirely sure that reporting a generic message like this is terribly
> useful anyway, honestly...  

So, maybe it's better to do not print anything at all. A new patch without PrintError() is coming.

Comment 10

[Andrea Marchesini [:baku]]

Attachment: throw.patch

I'm not sure if it makes sense to print something if the console API fails.
This patch removes the Throw() calls.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8412847 [details] [diff] [review]
throw.patch

In ProfileMethod, if ToObject() returned false we either need to throw on aRv or clear the pending exception.  Same for JS_DefineProperty and probably WrapJS.

If we do the throwing, we should also add reporting to ConsoleProfileRunnable::RunConsole, of course.

Similarly, in Console::Method I expect that a null return from CreateStack means an exception might be set.  So we really do need a ClearException there.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: interdiff

ConsoleProfileRunnable::RunConsole() has a ClearException object.
We can add the same object in Method() and ProfileMethod(). This should be enough.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: throw.patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8413654 [details] [diff] [review]
interdiff

r=me

Comment 15

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05babc6dd906

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05babc6dd906

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for uplift

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8413655 [details] [diff] [review]
throw.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 965860
User impact if declined: The console API can throw exceptions
Testing completed (on m-c, etc.): hard to reproduce.
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8413655 [details] [diff] [review]
throw.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 965860
User impact if declined: The console API can throw exceptions
Testing completed (on m-c, etc.): hard to reproduce.
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8413655 [details] [diff] [review]
throw.patch

Looks like this needs to be aurora & beta, flag error?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9f13a1af10c4
https://hg.mozilla.org/releases/mozilla-beta/rev/e295406432eb

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/e295406432eb