678327 - nsAttrValue::GetPtr should probably not allow getting non-pointer values as pointers

Status: RESOLVED WONTFIX

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

Description

[Benoit Jacob [:bjacob] (mostly away)]

This code in content/base/src/nsAttrValue.h:

inline void*
nsAttrValue::GetPtr() const
{
  NS_ASSERTION(BaseType() != eIntegerBase,
               "getting pointer from non-pointer");
  return reinterpret_cast<void*>(mBits & NS_ATTRVALUE_POINTERVALUE_MASK);
}

It seems that we shouldn't continue after such a "getting pointer from non-pointer". So NS_ASSERTION is not what we want here.

One thing stopping me from making a patch is that I don't understand the condition, BaseType() != eIntegerBase. Is eIntegerBase really the only non-pointer BaseType?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Benoit Jacob [:bjacob] from comment #0)
> It seems that we shouldn't continue after such a "getting pointer from
> non-pointer". So NS_ASSERTION is not what we want here.
What you mean here?
If the assertion fires, something is terribly wrong and should be
fixed.


> 
> One thing stopping me from making a patch is that I don't understand the
> condition, BaseType() != eIntegerBase. Is eIntegerBase really the only
> non-pointer BaseType?
Yes.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Olli Pettay [:smaug] from comment #1)
> (In reply to Benoit Jacob [:bjacob] from comment #0)
> > It seems that we shouldn't continue after such a "getting pointer from
> > non-pointer". So NS_ASSERTION is not what we want here.
> What you mean here?
> If the assertion fires, something is terribly wrong and should be
> fixed.

Exactly! That's why NS_ASSERTION is not what we want. Unless XPCOM_DEBUG_BREAK is set to a special value, NS_ASSERTION just prints a warning and continues, so in practice most of the time it's ignored, as we just have lots of noisy NS_ASSERTIONS all over the place.

For really bad things like this, when we hit this case and print this message, we probably also want to stop program execution. Like the standard assert macro. I've been told that MOZ_ASSERT does that...

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I'm ok with MOZ_ASSERT, though in the long run I'd just like us to
fix NS_ASSERTION usage.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Yes, MOZ_ASSERT seems like what we want:
http://mxr.mozilla.org/mozilla-central/source/mfbt/Util.h#73

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Are you talking about in release builds or just in debug builds?

If debug builds only, then what you're saying applies in every case in the whole tree where NS_ASSERTION is used.

My understanding has always been that the NS_ASSERTION macro should be used in cases where something is so horribly wrong that they should never happen. And if they do happen we can't expect the browser to keep working correctly.

The fact that the NS_ASSERTION macro doesn't abort execution right now is a bug IMHO. David Baron has been working on making that possible by trying to drive the number of assertions that fire during unit tests to 0. Once that is the case we can and should make NS_ASSERTION abort execution.

I'd much rather spend the time on getting there, than finding individual cases of NS_ASSERTIONs that don't fire during unit tests today and convert them to some new macro.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

> The fact that the NS_ASSERTION macro doesn't abort execution right now is a
> bug IMHO. David Baron has been working on making that possible by trying to
> drive the number of assertions that fire during unit tests to 0. Once that
> is the case we can and should make NS_ASSERTION abort execution.

Ah. I didn't realize that the recent discussion around asserts had taken this path. That's great news. If you think that this will happen in the foreseeable future then sure leave this NS_ASSERTION as is. If on the other hand, as I'm afraid, this would take long to happen, then maybe we're better off replacing this NS_ASSERTION by a MOZ_ASSERT.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

As long as I don't have to spend time reviewing the patch, feel free to change to MOZ_ASSERT anywhere you want ;-)

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't think we should run around and change random NS_ASSERTIONs to MOZ_ASSERTs.

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

Seems like the consensus is WONTFIX for this specific bug, but it will be solved anyways when the NS_ASSERTIONs become fatal.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

If someone knows the bug number for making NS_ASSERTIONs fatal, please make it block this.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

> The fact that the NS_ASSERTION macro doesn't abort execution right now is a
> bug IMHO. David Baron has been working on making that possible by trying to
> drive the number of assertions that fire during unit tests to 0. Once that
> is the case we can and should make NS_ASSERTION abort execution.

Ping! Do you have the bug number for this effort?

I've filed a tracking bug for places where NS_ASSERTION is used and its current default behavior is unsuitable: bug 682609.

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Bug 279923 / bug 404077?