Closed Bug 332328 Opened 18 years ago Closed 12 years ago

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

Categories

(Other Applications :: DOM Inspector, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: timeless, Unassigned)

References

()

Details

Attachments

(1 file, 4 obsolete files)

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]
Assignee: dom-inspector → nobody
QA Contact: timeless → dom-inspector
Is the return statement needed in the color setter? Or will be ok to just assign?
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #625441 - Flags: review?(Sevenspade)
Attached patch imrpoved patch (obsolete) — Splinter Review
This time returning a value whatever assignation is done.
Attachment #625441 - Attachment is obsolete: true
Attachment #625523 - Flags: review?(Sevenspade)
Attachment #625441 - Flags: review?(Sevenspade)
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.
Attachment #625523 - Flags: review?(Sevenspade) → review-
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.
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.
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.
Attached patch patch 2 (obsolete) — Splinter Review
Catching exceptions both on start and during runtime.
Attachment #625523 - Attachment is obsolete: true
Attachment #628043 - Flags: review?(Sevenspade)
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.
Attachment #628043 - Flags: review?(Sevenspade) → review-
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.)
Attached patch patch 3 (obsolete) — Splinter Review
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.
Attachment #628043 - Attachment is obsolete: true
Attachment #628782 - Flags: review?(Sevenspade)
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.
Attachment #628782 - Flags: review?(Sevenspade) → review+
Attached patch patch 4Splinter Review
Comment shortened so it fits into the 80 chars per line limit.
Attachment #628782 - Attachment is obsolete: true
Attachment #629213 - Flags: review?(Sevenspade)
Thank you for your explanation, Colby :-) I will take it into account in my next code, if needed.
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
Attachment #629213 - Flags: review?(Sevenspade) → review+
Pushed:
http://hg.mozilla.org/dom-inspector/rev/209f065e961d
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.