Closed Bug 1000947 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox30 + fixed
firefox31 + fixed
firefox32 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: billm, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → amarchesini
> 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?
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.
Blocks: 990789
Requesting tracking, since this can cause script running after the failing console call to randomly fail.
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.
(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!
Attached patch throw.patch (obsolete) — Splinter Review
Attachment #8412479 - Flags: review?(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.
Attachment #8412479 - Flags: review?(bzbarsky) → review-
> 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.
Attached patch throw.patch (obsolete) — Splinter Review
I'm not sure if it makes sense to print something if the console API fails.
This patch removes the Throw() calls.
Attachment #8412479 - Attachment is obsolete: true
Attachment #8412847 - Flags: review?(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.
Attachment #8412847 - Flags: review?(bzbarsky) → review-
Attached patch interdiffSplinter Review
ConsoleProfileRunnable::RunConsole() has a ClearException object.
We can add the same object in Method() and ProfileMethod(). This should be enough.
Attachment #8413654 - Flags: review?(bzbarsky)
Attached patch throw.patchSplinter Review
Attachment #8412847 - Attachment is obsolete: true
Attachment #8413655 - Flags: review?(bzbarsky)
Comment on attachment 8413654 [details] [diff] [review]
interdiff

r=me
Attachment #8413654 - Flags: review?(bzbarsky) → review+
Attachment #8413655 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/05babc6dd906
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Please nominate for uplift
Flags: needinfo?(amarchesini)
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
Attachment #8413655 - Flags: approval-mozilla-aurora?
Flags: needinfo?(amarchesini)
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
Attachment #8413655 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8413655 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8413655 [details] [diff] [review]
throw.patch

Looks like this needs to be aurora & beta, flag error?
Attachment #8413655 - Flags: approval-mozilla-aurora+
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.