1838380 - Strange 0 (nullptr) reference. We should use proper abort mechanism. mailnews/db/mork/morkDeque.h

Status: RESOLVED WORKSFORME

(MailNews Core :: Database, defect)

Description

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: Use proper run-time assert instead of accessing a memory via null pointer.

There is a code in morkDeque.h which references a pointer value of 0.
Intention seems to be the code should crash when the program calls this function.

https://searchfox.org/comm-central/source/mailnews/db/mork/morkDeque.h#65

I am using GCC to compile TB under Debian GNU/Linux locally.
GCC has become very clever and when it sees it, the compilation fails since the code is buggy.

I think we should use proper abort mechanism such as MORK_ASSERT(0);

Come to think of it, I am not sure why GCC does not complain about |MORK_ASSERT(0)| at compile time, but I suspect there have been enough efforts to placate the compier by using enough indirection or proper pragma magic so that the compiler ignores this always fail condition in |MORK_ASSERT(0)|.

I am not sure whom I should ask the review.
Bugzilla doesn't suggest a potential reviewer.

Comment 1

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Wayne, maybe you can assign a proper reviewer?

Comment 2

[Wayne Mery (:wsmwk)]

Or maybe Ben

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9339012 [details] [diff] [review]
Use proper run-time assert instead of accessing a memory via null pointer.

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

Please don't add a #if 0 version, just a change with the fix.
Also, please use moz-phab for submitting patches. https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html.

Comment 4

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to Magnus Melin [:mkmelin] from comment #3)

Comment on attachment 9339012 [details] [diff] [review]
Use proper run-time assert instead of accessing a memory via null pointer.

Review of attachment 9339012 [details] [diff] [review]:

Please don't add a #if 0 version, just a change with the fix.
Also, please use moz-phab for submitting patches.
https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html.

All right. I have not used phabricator submission before, but I will study the document.

Comment 5

[Ben Campbell]

I'm constantly surprised by Mork :-)

The change looks good to me. As Magnus said, no point leaving in the old code. You know your stuff, so be bold!

Comment 6

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

(In reply to Ben Campbell from comment #5)

I'm constantly surprised by Mork :-)

The change looks good to me. As Magnus said, no point leaving in the old code. You know your stuff, so be bold!

Yes, I will submit via phabricator after proper mod of the patch.
Except, due to the busy day job and other commitments, I have realized I have not moved away from hg MQ practice YET.
I did play with evolution, etc. So hopefully in about a week, I can submit the updated patch via phabricator.
This is a very easy patch, a single chunk to a single file. So a good practice for me to use phabricator.

Comment 7

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Attachment: Bug 1838380 - use proper abort macro, r?mkmelin

Comment 8

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

Hmm, I must have screwed up something.
The patch which I tried to submit via moz-phab ended up as
https://phabricator.services.mozilla.com/D184904
and not shown in this bugzilla.

(This is in contrast to the patch which DID show up in the bugzilla, Bug 673703
https://phabricator.services.mozilla.com/D184903

Comment 9

[Magnus Melin [:mkmelin]]

I'm not seeing any problem with the submission. D184904 is listed in this bug as it should.

Comment 10

[ISHIKAWA, Chiaki (back from day job exhibition event.)]

I am a bit hazy what happened, but
we changed MOZ_ASSERT() into MOZ_ASSERT_UNREACHABLE() and it has been fixed that way.
So I am going to close this.

Comment 11

[Magnus Melin [:mkmelin]]

We use FIXED only when known patches resolved the issue.