Closed
Bug 1000947
Opened 11 years ago
Closed 11 years ago
Console::Method can throw, but it's not annotated as [Throws] in WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: billm, Assigned: baku)
References
(Blocks 1 open bug)
Details
(Whiteboard: [qa-])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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 | ||
Updated•11 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 1•11 years ago
|
||
> 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•11 years ago
|
||
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
Comment 3•11 years ago
|
||
Requesting tracking, since this can cause script running after the failing console call to randomly fail.
tracking-firefox30:
--- → ?
tracking-firefox31:
--- → ?
Reporter | ||
Comment 4•11 years ago
|
||
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•11 years ago
|
||
(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•11 years ago
|
||
Indeed. Filed bug 1001157.
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #8412479 -
Flags: review?(bzbarsky)
Comment 8•11 years ago
|
||
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-
Assignee | ||
Comment 9•11 years ago
|
||
> 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.
Assignee | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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-
Updated•11 years ago
|
Assignee | ||
Comment 12•11 years ago
|
||
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)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8412847 -
Attachment is obsolete: true
Attachment #8413655 -
Flags: review?(bzbarsky)
Comment 14•11 years ago
|
||
Comment on attachment 8413654 [details] [diff] [review]
interdiff
r=me
Attachment #8413654 -
Flags: review?(bzbarsky) → review+
Updated•11 years ago
|
Attachment #8413655 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Comment 16•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 17•11 years ago
|
||
Please nominate for uplift
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox32:
--- → fixed
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 18•11 years ago
|
||
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)
Assignee | ||
Comment 19•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #8413655 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•11 years ago
|
||
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+
Comment 21•11 years ago
|
||
Comment 22•11 years ago
|
||
Updated•11 years ago
|
status-b2g-v1.4:
--- → fixed
Whiteboard: [qa-]
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•