http://code.google.com/p/chromium/issues/detail?id=172416 console.debug() causes a NOTREACHED() assert in content::RenderViewImpl::didAddMessageToConsole. WebCore::Console:debug calls Webcore::internalAddMessage with a MessageLevel of DebugMessageLevel. This makes it through to Webkit::ChromeClientImpl::addMessageToConsole, which casts it from WebCore::MessageLevel to WebConsoleMessage::Level. WebCore::MessageLevel (third_party/WebKit/Source/WebCore/page/ConsoleTypes.h) is an enum of 5 levels: enum MessageLevel { TipMessageLevel, LogMessageLevel, WarningMessageLevel, ErrorMessageLevel, DebugMessageLevel }; WebConsoleMessage::Level (third_party/WebKit/Source/WebKit/chromium/public/WebConsoleMessage.h) is an enum of only 4 levels: enum Level { LevelTip, LevelLog, LevelWarning, LevelError };
Created attachment 184858 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 184858 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184858&action=review > Source/WebKit/chromium/public/WebConsoleMessage.h:44 > - LevelError > + LevelError, > + LevelDebug Do we have entries in AssertMachingEnums for these?
We do not. We should. Kevin, would you mind adding the necessary assertions to Source/WebKit/chromium/src/AssertMatchingEnums.cpp (and, if you're feeling generous, the other ports as well?) as part of this patch?
(In reply to comment #3) > (From update of attachment 184858 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184858&action=review > > > Source/WebKit/chromium/public/WebConsoleMessage.h:44 > > - LevelError > > + LevelError, > > + LevelDebug > > Do we have entries in AssertMachingEnums for these? Source/WebKit/chromium/src/AssertMatchingEnums.cpp makes no mention of message levels, or anything to do with the console. It's also probably safe to add to this enum without ensuring it matches, since out-of-bounds enum values are already getting passed into the console renderer every time console.debug is being called. Without debug assertions it's getting silently converted to logging::LOG_VERBOSE, so this ends up being a no-op and just documenting better what's actually happening. I'm happy to add a test if you guys want though?
(In reply to comment #4) > We do not. We should. > > Kevin, would you mind adding the necessary assertions to Source/WebKit/chromium/src/AssertMatchingEnums.cpp (and, if you're feeling generous, the other ports as well?) as part of this patch? Sure thing, I will update the patch in the morning after I have a chance to test it.
(In reply to comment #6) > (In reply to comment #4) > > We do not. We should. > > > > Kevin, would you mind adding the necessary assertions to Source/WebKit/chromium/src/AssertMatchingEnums.cpp (and, if you're feeling generous, the other ports as well?) as part of this patch? > > Sure thing, I will update the patch in the morning after I have a chance to test it. If dealing with the other ports ends up being more work than you signed up for (especially if they also don't have the debug-level enum), I'll take care of them. I do think that adding asserts to the Chromium port should be part of this patch, however, so I appreciate you looking into it. Thank you!
Created attachment 184872 [details] Patch
I checked for similar problems in other platforms: blackberry: The only place MessageLevel is referenced is in ChromeClientBlackBerry::addMessageToConsole, where that parameter is completely ignored. There is no blackberry specific enum for message levels. Blackberry also doesn't have an AssertMatchingEnums. chromium: Updated AssertMatchingEnums.cpp to test these enums. efl: The only place MessageLevel is referenced is in ChromeClientEfl::addMessageToConsole, where that parameter is completely ignored. There is no efl specific enum for message levels. efl does have an AssertMatchingEnums, but there is no webkit levels enum to add to it. gtk: exactly the same as efl mac: The only place MessageLevel is referenced is in WebChromeClient::addMessageToConsole. It is actually used here, but there is no matching enum - the only thing that's done with it is to call stringForMessageLevel. qt: exactly the same as efl, except that it has no AssertMatchingEnums win: exactly the same as efl, except that it has no AssertMatchingEnums wince: exactly the same as efl, except that it has no AssertMatchingEnums wx: MessageLevel is used in ChromeClientWx::addMessageToConsole, and is cast to static_cast<WebViewConsoleMessageLevel>(level) similar to how Chromium handles this. WebViewConsoleMessageLevel is also missing a DebugMessageLevel, which is an easy fix. I don't have a wx development environment set up though, so I'm not sure if anything higher up would need to be patched to allow for this. Nothing in WebKit/wx actually ever uses the level. It's stuffed into an m_level variable inside WebViewConsoleMessageEvent, but nothing inside WebKit uses that. I don't know if this is safe to change or not. TL;DR: Everything except wx looks okay, but I don't have enough familiarity with wx to know if it's safe to change. I updated my patch to include an AssertMatchingEnums test for chromium.
Comment on attachment 184872 [details] Patch do you have a chromium patch ready as well?
*** Bug 108039 has been marked as a duplicate of this bug. ***
(In reply to comment #10) > (From update of attachment 184872 [details]) > do you have a chromium patch ready as well? (if not, I have https://codereview.chromium.org/12091015)
Comment on attachment 184872 [details] Patch API change LGTM
(In reply to comment #12) > (In reply to comment #10) > > (From update of attachment 184872 [details] [details]) > > do you have a chromium patch ready as well? > > (if not, I have https://codereview.chromium.org/12091015) That is basically identical to my unsubmitted patch, so feel free to use that.
Comment on attachment 184872 [details] Patch Clearing flags on attachment: 184872 Committed r141006: <http://trac.webkit.org/changeset/141006>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 108123
http://trac.webkit.org/changeset/141234