332328 - WARNING: NS_ENSURE_TRUE(!(aColor.IsEmpty())) failed: file r:/mozilla/extensions/inspector/base/src/inFlasher.cpp, line 88

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[timeless]

This ensure seems to indicate that set color should only set .color if aVal isn't empty.

00 ntdll!DbgBreakPoint (FPO: [0,0,0])
01 xpcom_core!Break(char * aMsg = 0x00124230 "WARNING: NS_ENSURE_TRUE(!(aColor.IsEmpty())) failed: file r:/mozilla/extensions/inspector/base/src/inFlasher.cpp, line 88")+0x203 (FPO: [Non-Fpo]) (CONV: cdecl) [r:\mozilla\xpcom\base\nsdebugimpl.cpp @ 475]
02 xpcom_core!NS_DebugBreak_P(unsigned int aSeverity = 0, char * aStr = 0x02cea344 "NS_ENSURE_TRUE(!(aColor.IsEmpty())) failed", char * aExpr = 0x00000000 "", char * aFile = 0x02cea30c "r:/mozilla/extensions/inspector/base/src/inFlasher.cpp", int aLine = 88)+0x2ec (FPO: [Non-Fpo]) (CONV: cdecl) [r:\mozilla\xpcom\base\nsdebugimpl.cpp @ 358]
03 inspector!inFlasher::SetColor(class nsAString_internal * aColor = 0x050d1870)+0x2b (FPO: [Non-Fpo]) (CONV: stdcall) [r:\mozilla\extensions\inspector\base\src\inflasher.cpp @ 88]
04 xpcom_core!XPTC_InvokeByIndex(class nsISupports * that = 0x0599e648, unsigned int methodIndex = 4, unsigned int paramCount = 1, struct nsXPTCVariant * params = 0x0012482c)+0x27 (CONV: cdecl) [r:\mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp @ 102]
05 xpc3250!XPCWrappedNative::CallMethod(class XPCCallContext * ccx = 0x001249f4, XPCWrappedNative::CallMode mode = CALL_SETTER (2))+0xddf (FPO: [Non-Fpo]) (CONV: cdecl) [r:\mozilla\js\src\xpconnect\src\xpcwrappednative.cpp @ 2155]
06 xpc3250!XPCWrappedNative::SetAttribute(class XPCCallContext * ccx = 0x001249f4)+0xe (FPO: [Non-Fpo]) (CONV: cdecl) [r:\mozilla\js\src\xpconnect\src\xpcprivate.h @ 1987]
07 xpc3250!XPC_WN_GetterSetter(struct JSContext * cx = 0x0364faa8, struct JSObject * obj = 0x04ba7238, unsigned int argc = 1, long * argv = 0x05d02cb0, long * vp = 0x00124b04)+0x1fe (FPO: [Non-Fpo]) (CONV: cdecl) [r:\mozilla\js\src\xpconnect\src\xpcwrappednativejsops.cpp @ 1495]

0 [native frame]
1 Flasher(aColor = null, aThickness = null, aDuration = null, aSpeed = null, aInvert = null) ["chrome://inspector/content/Flasher.js":58]
    this = [object Object]
2 anonymous() ["chrome://inspector/content/viewers/dom/dom.js":697]
    this = [object Object]
3 vr_listprops() ["chrome://venkman/content/venkman-records.js":862]
    i = undefined
    jsval = [object Object]
    propMap = [object Object]
    p = "flasher"
    value = [xpconnect wrapped jsdIValue @ 0x5841ad0 (native @ 0x5abb230)]
    localProps = undefined
    len = undefined
    prop = undefined
    name = undefined
    nameList = undefined
    propertyList = undefined
    this = [object Object]
4 vr_preopen() ["chrome://venkman/content/venkman-records.js":990]
    rec = undefined
    i = undefined
    prop = undefined
    this = [object Object]
5 xtvr_open() ["chrome://venkman/content/tree-utils.js":839]
    delta = undefined
    i = undefined
    this = [object Object]
6 xtv_toggleopen(index = 2) ["chrome://venkman/content/tree-utils.js":1352]
    row = [object Object]
    this = [object Object]
7 [native frame]
8 changeOpenState(row = 2, openState = true) ["chrome://global/content/bindings/tree.xml":221]
    event = undefined
    this = [object XULElement @ 0x4332690 (native @ 0x3bbeed8)]
9 onxblkeypress(event = [object KeyboardEvent @ 0x55d0af0 (native @ 0x5c8cd90)]) ["chrome://global/content/bindings/tree.xml":268]
    row = 2
    view = undefined
    this = [object XULElement @ 0x4332690 (native @ 0x3bbeed8)]
10 [native frame]

Comment 1

[Javi Rueda :javirid]

Is the return statement needed in the color setter? Or will be ok to just assign?

Comment 2

[Javi Rueda :javirid]

Attachment: patch

Checks for the value not being null or empty and and if it is ok, then sets color. Don't know it it is necessary to return the value, so I just made this change to the code.

Comment 3

[Javi Rueda :javirid]

Attachment: imrpoved patch

This time returning a value whatever assignation is done.

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 625523 [details] [diff] [review]
imrpoved patch

Review of attachment 625523 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is the right fix for this bug.

WARNING: NS_ENSURE_TRUE(!(aColor.IsEmpty())) failed: file ../../../../layout/inspector/src/inFlasher.cpp, line 83
JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [inIFlasher.color]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://inspector/content/Flasher.js :: Flasher :: line 25"  data: no]

Things are a little different from the way they were at the filing date, but I did some source archaeology and there's enough detail in comment #0 to guess what happened.  In this case, NS_ENSURE_FALSE is doing what it should: warning about a case that shouldn't occur.

We can see from the stack trace that aColor is null, along with the rest of the arguments.  These are being supplied by PrefUtils's getPref, which will return null if the pref branch get methods fail (unlikely, and even so, means that the pref branch code is buggy), or if the preferences don't exist (more likely the case).

I'm not sure of the state of things in 2006, but today toolkit preference defaults and the inability to remove them mean that this should only happen with a corrupted profile, or some other toolkit bug that would cause the preferences not to be read in at startup.

That's a case where we want some indication that something has gone wrong, rather than silently routing around it.

Having said that, it's not the conditions of the original bug as filed, but there's another case where this warning would be emitted: if you were to use about:config to change the preference in question to the empty string.

Both cases are recoverable (though it depends on how "recoverable" you view a corrupted profile), so in that regard, your patch does adequate recovery, except the silent fallback gives no signal that something is wrong.

If you submit a patch that does that, I'll accept it.

Comment 5

[Javi Rueda :javirid]

Thank you for job, Colby.

Which kind of signal is needed? An alert in the Error Console would be ok?

I am assuming that last patch is ok, but needs to alert about the case where aVal==empty.

Comment 6

[Colby Russell :crussell (no longer Mozilla-ing)]

The problem here, really, is that the exception goes uncaught (initialization fails, and the window is unusable).  So simply catching the exception and reporting it would work.  You can use Components.utils.reportError.  This would also fail-safe against another case: when the pref has a non-empty value, but where that value isn't a valid RGB hex triple.

Comment 7

[Javi Rueda :javirid]

Added a try/catch block in the class constructor Flasher, so now we will catch the exception thrown in the case that the color appearing in about:config is invalid or empty.

Added another try/catch block in the color setter, so when an invalid or empty value is passed to the function, it will be thrown an exception.

Some test-cases I ran:

* Open DOMi with default values in about:config. Then setting border-color to empty string. Exception thrown. Previously read default value is used at runtime

* Setting border-color to blank string and closing DOMi. When opening it again, an exception is thrown and default color value (#000000) is used at runtime.

Test-cases above seem ok to me.

Some other test-cases I ran which I am not sure that worked as expected:

* Opening with non-empty but invalid value (like "#cc000") in border-color preference. Exception thrown and using #000000. Then, setting blink-color to an empty string (so invalid value too). It doesn't throw an exception and defaults to "#000000".

So. If there is an invalid value in the prefs (including the blank string), #000000 will be used as the border-color.

Comment 8

[Javi Rueda :javirid]

Attachment: patch 2

Catching exceptions both on start and during runtime.

Comment 9

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 628043 [details] [diff] [review]
patch 2

Review of attachment 628043 [details] [diff] [review]:
-----------------------------------------------------------------

::: resources/content/Flasher.js
@@ +26,5 @@
> +    this.mShell.color = aColor;
> +  }
> +  catch(e) { // Catching the exception in case the color pref value is invalid.
> +    Components.utils.reportError(e);
> +  }

Please just use |this.color = aColor| (like duration below).  With the try/catch in the color setter, this one is unnecessary.

@@ +71,5 @@
> +      this.mShell.color = aVal;
> +    }
> +    catch(e) {
> +      Components.utils.reportError(e);
> +      this.mShell.color = this.mShell.color;

What's this line supposed to do?

@@ +73,5 @@
> +    catch(e) {
> +      Components.utils.reportError(e);
> +      this.mShell.color = this.mShell.color;
> +    }
> +    return this.mShell.color;

|return aVal|, please.  JS semantics usually have the result of an assignment be the rval, even when the assignment fails.

Comment 10

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 628043 [details] [diff] [review]
patch 2

Review of attachment 628043 [details] [diff] [review]:
-----------------------------------------------------------------

::: resources/content/Flasher.js
@@ +69,5 @@
> +  {
> +    try {
> +      this.mShell.color = aVal;
> +    }
> +    catch(e) {

Nit: Please also put a space before the open parenthesis.  (Compare to other control-flow statements like if and while.)

Comment 11

[Javi Rueda :javirid]

Attachment: patch 3

Some changes to previous patch:

Using only one try/catch because the block at the constructor was no needed.

Trying to assign to this.mShell.color the value of itself was not needed as it will keep it.

Comment 12

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 628782 [details] [diff] [review]
patch 3

Review of attachment 628782 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Javi Rueda from comment #7)
> Added a try/catch block in the class constructor Flasher, so now we will
> catch the exception thrown in the case that the color appearing in
> about:config is invalid or empty.
> 
> Added another try/catch block in the color setter, so when an invalid or
> empty value is passed to the function, it will be thrown an exception.

I thought I would explain why that was unnecessary:
When you use a try/catch and do Components.utils.reportError in the catch body, that's not the same as re-throwing the exception.  reportError logs it to the console, but the catch otherwise swallows the exception.  The exception stops propogating upward to the callers, in the same way that it would stop if you had some code like:

try {
  // This will throw if fum was already initialized.
  fum.init();
}
catch (ex) {
  // It didn't need to be initialized.  Carry on, then.
}

::: resources/content/Flasher.js
@@ +64,5 @@
> +  {
> +    try {
> +      this.mShell.color = aVal;
> +    }
> +    catch (e) { // Catch the exception in case aVal is an invalid or empty value.

Line length here. r=me with that fixed.

Comment 13

[Javi Rueda :javirid]

Attachment: patch 4

Comment shortened so it fits into the 80 chars per line limit.

Comment 14

[Javi Rueda :javirid]

Thank you for your explanation, Colby :-) I will take it into account in my next code, if needed.

Comment 15

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 629213 [details] [diff] [review]
patch 4

Review of attachment 629213 [details] [diff] [review]:
-----------------------------------------------------------------

When you get r+ on a patch on the condition that you change something, after you change it, you don't need to get review on a new patch that includes that change (unless you want to).  If you have commit access, you can just fix it and commit it.  Otherwise, you can just attach a new patch, mark it as r+, and add checkin-needed to the whiteboard.

https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Addressing_review_comments

Comment 16

[Colby Russell :crussell (no longer Mozilla-ing)]

Pushed:
http://hg.mozilla.org/dom-inspector/rev/209f065e961d