Closed Bug 401755 Opened 14 years ago Closed 14 years ago

Cleanup nsNSSIOLayer.cpp->getErrorMessage

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: johnath, Assigned: KaiE)

References

Details

Attachments

(4 files, 2 obsolete files)

As a follow-up to the changes made in bug 398718 BobR requested (comment 47) that the getErrorMessage code be cleaned up.

> Also, the block is a very long code block with lots of extra conditionals
> embedded in a middle of a switch statement for an alreadly very long function.
> It's crying out to be broken up into at least one or two extra functions
> (making getting the SAN it's own function would greatly simplify the logic and
> get rid of lots of if (useSAN) blocks.
moved here, from bug 398718 comment 57


Bob, here is the requested cleanup you requested.

This patch is a nightmare to review, because it contains moved code and change
of indenting.

Because of that I will attach a separate patch that displays the changes much
more easily.
Attached patch Patch v1 (reviewer friendly) (obsolete) — Splinter Review
moved here, from bug 398718 comment 58


This patch illustrates what I'm really doing:

- moved code to separate 4 functions that each append some text

- declare some more local helper variables, where needed

- enhanced the code that produces the expiration error message.

  If we're unable to format the time, we should not abort without a message.
  We should still display the correct wording, and simply leave the
  time value empty.


Note, in this special version of the patch:
- the new functions are below getInvalidCertErrorMessage, 
  so the order of the code is kept, and therefore the diff is minimal 
- added forward declarations to make it compile
- ignores whitespace changes

But I really want to check in the other version of the patch,
which has correct whitespace and has the new functions before
getInvalidCertErrorMessage, so the forward declarations are not necessary.
Attachment #286876 - Flags: review?(rrelyea)
Comment on attachment 286875 [details] [diff] [review]
Patch v1  (see next attachment for easier reviewing)

r-

I like the separate functions, but my code that raised the issue with me was code structured:


PRBool myBool= PR_TRUE;

if (myBool) {
    rv = doSomething();
    if (!rv) {
        myBool = PR_FALSE;
    }
  }
  if (myBool) {
     rv = doSomethingElse();
     if (!rv) {
       myBool = PR_FALSE;
     }
  }


It makes the code hard to read when what you really want is

func1() {
    rv = doSomething();
    if (!rv) { 
         return rv;
    }
    rv = doeSomething2();
    if (!rv) {
          return rv;
    }
    .
    .
    .
    return OK;
}

    rv = func1();
    if (rv) {
      finish1();
   } else {
      finish2();
   }


 or equivalent. Not only did you keep the old version with this structure, you added a new version with the same structure.

bob
Attachment #286875 - Flags: review-
Attachment #286876 - Flags: review?(rrelyea)
(In reply to comment #3)
> 
> I like the separate functions, but my code that raised the issue with me was
> code structured:

Ok, I see.

Well, my patch is not exactly what you describe. Because I don't really want to return. The end of the function contains an action that depends on the earlier code. So, your request required me to completely restructure the two functions.

Patch coming up.
Attached patch Patch v2Splinter Review
Attachment #286875 - Attachment is obsolete: true
Attachment #286876 - Attachment is obsolete: true
Attachment #286983 - Flags: review?(rrelyea)
Comment on attachment 286983 [details] [diff] [review]
Patch v2 (reviewer friendly, ignores whitespace)

Bob, can you please review?
Comment on attachment 286982 [details] [diff] [review]
Patch v2

r+ = rrelyea

that's much better. Much easier to read and see what is going on.

bob
Attachment #286982 - Flags: review+
Comment on attachment 286983 [details] [diff] [review]
Patch v2 (reviewer friendly, ignores whitespace)

r+ the original.

removing review request on reviewer friendly..
Attachment #286983 - Flags: review?(rrelyea)
Attachment #286982 - Flags: approval1.9?
Attachment #286982 - Flags: approval1.9? → approval1.9+
This patch differs from the reviewed patch in the following minor details:

- one string ID got renamed by a different check in

- I moved the separate functions further up in the file, so the forward declarations have become unnecessary (as announced I would do, earlier in this bug)
fixed
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
This patch introduced the use of an uninitialized variable.  Var "trueExpired_falseNotYetValid" is defined and immediately used on the folowing line:

http://mxr.mozilla.org/firefox/source/security/manager/ssl/src/nsNSSIOLayer.cpp#910

(In reply to comment #12)
> This patch introduced the use of an uninitialized variable.  Var
> "trueExpired_falseNotYetValid" is defined and immediately used on the folowing
> line:


Thanks. It's a real regression, introduced by the cleanup...

The variable is supposed to be used as an in/out parameter, and therefore param 3 of GetDateBoundary must be changed to be a reference.

Reopening to get this fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #288878 - Flags: review?(rrelyea)
Attachment #288878 - Flags: review?(rrelyea) → review+
Attachment #288878 - Flags: approval1.9?
Attachment #288878 - Flags: approval1.9? → approval1.9+
patch v3 checked in, marking fixed again
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.