Closed Bug 1061521 Opened 5 years ago Closed 4 years ago

Throw Component.exceptions instead of strings in about:memory

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: Yoric, Assigned: rutuja.r.surve, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(3 files, 13 obsolete files)

3.22 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
15.93 KB, application/vnd.mozilla.xul+xml
Details
2.00 KB, patch
mconley
: review+
Details | Diff | Splinter Review
The objective of this bug is to fix many of the throwing statements of directory toolkit/components/aboutmemory (see http://dxr.mozilla.org/mozilla-central/search?q=throw+path%3Atoolkit%2Fcomponents%2Faboutmemory&case=true ).

Generally, in JavaScript, you can call `throw foo` to throw an error, but if `foo` is not an instance of `Error`, this complicates debugging a lot. So, we want to look at all the calls to `throw foo` in that directory and make sure that we always throw instances of `Error`.

- wherever there is `throw "Some text"`, replace it with `throw new Error("Some text")`;
- wherever there is `throw Components.results.SOME_CONSTANT` or equivalently `throw Cr.SOME_CONSTANT`, replace it with `throw new Components.Exception(some message, Components.results.SOME_CONSTANT)`.
Whiteboard: [lang=js][good first bug]
I would like to patch this bug. Also it is my first bug so guide me through.
Hello, Alucard! The first thing to do is get your development environment running. This is a good place to start:

https://developer.mozilla.org/en-US/docs/Introduction 

Once you're set up to work on Firefox, you should be able start working on a patch to the files David links to, above.
Hey David, okay to take this? It seems I'm late here too :(
David, are there any similar bugs that Jeffrey could take?
Flags: needinfo?(dteller)
Hey, I have build up the Firefox.Please help me further. Thanks.
(In reply to alucardgabriel1 from comment #5)
> Hey, I have build up the Firefox.Please help me further. Thanks.

Modify the relevant files (see comment 0 for a description of how to find them). Then rebuild Firefox, start it, and visit "about:memory". Hopefully things should work ok. If you've introduced any errors, they'll show up in the console, which you can view with Ctrl-Shift-J.
(In reply to Nicholas Nethercote [:njn] from comment #6)
> (In reply to alucardgabriel1 from comment #5)
> > Hey, I have build up the Firefox.Please help me further. Thanks.
> 
> Modify the relevant files (see comment 0 for a description of how to find
> them). Then rebuild Firefox, start it, and visit "about:memory". Hopefully
> things should work ok. If you've introduced any errors, they'll show up in
> the console, which you can view with Ctrl-Shift-J.

Can you explain more
I've given you some reasonably detailed and specific instructions. You've given me vague requests for more help. You're going to have to be more specific about what it is that you're having trouble with. It might be worth joining the #introduction channel on IRC (http://irc.mozilla.org/).
Hi Nicholas, I've changed the code, I built it again and run it. I din't find any errors in the console. What should i do next? How to make a patch?
I've made changes as described. I found no errors. Can you please check if it is ok.
Attachment #8484745 - Flags: review?(dteller)
Sorry vamsivarikuti, didn't see you had attached a patch. Ooops
Oh dear. Two patches within five minutes of each other, that's unfortunate. On first glance both patches look like they need some things fixed, so I'll review both and then decide what to do.
Comment on attachment 8484745 [details] [diff] [review]
bug-1061521-fix.patch: throwing exception instance instead of plain string

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

A good first try! But there's a few things that need fixing.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +90,5 @@
>  {
>    let str = ex.toString();
>    if (str.startsWith(gAssertionFailureMsgPrefix)) {
>      // Argh, assertion failure within this file!  Give up.
> +    throw new Error(ex);

|ex| is already an exception object, so this change isn't necessary.

@@ +627,5 @@
>        },
>        onStopRequest: function(aR, aC, aStatusCode) {
>          try {
>            if (!Components.isSuccessCode(aStatusCode)) {
> +            throw new Error(aStatusCode);

|aStatusCode| is a number... I'm not sure if this is the right way to treat this. Yoric will know.

::: toolkit/components/aboutmemory/tests/test_aboutmemory.xul
@@ +130,5 @@
>    // exception.
>    try {
>      is(mgr.explicit, 500*MB + (100 + 13 + 10)*MB + 599*KB, "mgr.explicit");
>    } catch (ex) {
> +    is(ex.result, new Components.Exception(Components.results.NS_ERROR_NOT_AVAILABLE), "mgr.explicit exception");

That's not right. |is| is a function that compares its first two args for equality and causes the test to fail if it fails. So you've changed the meaning of that comparison.

If you run this test (with a command like |mach mochitest-chrome toolkit/components/aboutmemory/tests/test_aboutmemory.xul| it will now fail.

::: toolkit/components/aboutmemory/tests/test_memoryReporters.xul
@@ +145,5 @@
>    let haveExplicit = true;
>    try {
>      dummy = mgr.explicit;
>    } catch (ex) {
> +    is(ex.result, new Components.Exception(Components.results.NS_ERROR_NOT_AVAILABLE), "mgr.explicit exception");

Ditto.

@@ +176,5 @@
>        dummy = mgr[amounts[i]];
>        ok(dummy !== undefined,
>           "accessed an unknown distinguished amount: " + amounts[i]);
>      } catch (ex) {
> +      throw new Exception(ex);

That's not right. Read the comment above, particularly the "just move on" part.
Attachment #8484745 - Flags: feedback+
Comment on attachment 8484748 [details] [diff] [review]
Throw Component.exceptions instead of strings

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

Also a good try, with a few things that need fixing.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +76,5 @@
>  function assert(aCond, aMsg)
>  {
>    if (!aCond) {
>      reportAssertionFailure(aMsg)
> +    throw Components.Exception(gAssertionFailureMsgPrefix + aMsg);

|gAssertionFailureMsgPrefix + aMsg| is a string, so as per comment 0 you just want |throw new Error(gAssertionFailureMsgPrefix + aMsg);|.

@@ +84,5 @@
>  // This is used for malformed input from memory reporters.
>  function assertInput(aCond, aMsg)
>  {
>    if (!aCond) {
> +    throw Components.Exception("Invalid memory report(s): " + aMsg);

Ditto.

@@ +93,5 @@
>  {
>    let str = ex.toString();
>    if (str.startsWith(gAssertionFailureMsgPrefix)) {
>      // Argh, assertion failure within this file!  Give up.
> +    throw Components.Exception(ex);

|ex| is already an exception object, so this change isn't necessary.

@@ +644,5 @@
>        },
>        onStopRequest: function(aR, aC, aStatusCode) {
>          try {
>            if (!Components.isSuccessCode(aStatusCode)) {
> +            throw Components.Exception(aStatusCode);

I'm not sure about this one. Yoric will know.
Attachment #8484748 - Flags: feedback+
Since both vamsivarikuti and Jeffrey have done decent first attempts, we're going to have to make an arbitrary decision about who finishes off the bug. Jeffrey has had other people get in front of him in at least one other bug, so I'll ask him to continue.

Sorry about that, vamsivarikuti! I know this must be frustrating. Having two people submit patches like this so close to each other is quite unusual. Do you have another bug that you could work on? Perhaps Mike Hoye could suggest one...
Flags: needinfo?(mhoye)
Attachment #8484748 - Attachment is obsolete: true
Attachment #8484748 - Flags: review?(dteller)
Attachment #8484760 - Flags: review?(dteller)
Attachment #8484760 - Flags: feedback?(n.nethercote)
Comment on attachment 8484760 [details] [diff] [review]
Throw Component.exceptions or errors instead of strings

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

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +84,5 @@
>  // This is used for malformed input from memory reporters.
>  function assertInput(aCond, aMsg)
>  {
>    if (!aCond) {
> +    throw Components.Exception("Invalid memory report(s): " + aMsg);

Should be |new Error|.
Attachment #8484760 - Flags: feedback?(n.nethercote)
Vamsivarikuti, would you be interested in picking up bug 1064821 instead? This is a very similar bug.
Flags: needinfo?(dteller) → needinfo?(vamsivarikuti)
Comment on attachment 8484778 [details] [diff] [review]
Throw Component.exceptions or errors instead of strings

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

This looks good to me. Just a little change: your commit message should end with ",r=yoric", to make it easier to find out who reviewed the patch.

Let's see if it works as expected:
https://tbpl.mozilla.org/?tree=Try&rev=f1845cbb52ec
Attachment #8484778 - Flags: review?(dteller) → review+
Comment on attachment 8484745 [details] [diff] [review]
bug-1061521-fix.patch: throwing exception instance instead of plain string

As mentioned by Nicholas, it's a shame both patches were submitted at the same time. Since Jeffrey was already victim of another collision in another bug, would you accept moving to another related bug?
Attachment #8484745 - Flags: review?(dteller)
Changed the commit message to "Bug 1061521 - Throw Component.exceptions instead of strings, r=yoric"
Attachment #8484778 - Attachment is obsolete: true
Attachment #8486401 - Flags: review+
According to unit testing, this causes some tests to fail. Interesting.

I believe that part of the issue is here: http://dxr.mozilla.org/mozilla-central/source/toolkit/components/aboutmemory/content/aboutMemory.js#92. This line assumes that the exception is a string, which is quite ugly. Let's go for the simple fix, for the moment, and use `ex.message` instead of `ex.toString()`. We might wish to go for something cleaner in a followup bug, later.

This should solve some of the errors. I believe that toolkit/components/aboutmemory/tests/test_aboutmemory3.xul and toolkit/components/aboutmemory/tests/test_aboutmemory4.xul are also hardcoded with the exact string that is currently thrown. You might also need to find out what to change exactly in these tests to fix the issue.

To run these particular tests on your computer, use
  ./mach build toolkit/components/aboutmemory
(to rebuild Firefox)
 ./mach mochitest toolkit/components/aboutmemory/tests/test_aboutmemory3.xul
(to actually launch the test)
David: Yes, I am interested. Could you please provide me links to documentation of this project.
Flags: needinfo?(vamsivarikuti) → needinfo?(dteller)
vamsivarikuti: just follow the link in comment 20
Flags: needinfo?(dteller)
Flags: needinfo?(mhoye)
Yoric: What is the status of this bug?
Flags: needinfo?(dteller)
Jeffrey, any news?
Flags: needinfo?(dteller) → needinfo?(jeffgodwyll)
Attached patch bug1061521.diff (obsolete) — Splinter Review
Was away. Back now. Not sure what to change in those test cases so the "pasted text" error doesn't get thrown, but new change causes nothing to fail anymore
Flags: needinfo?(jeffgodwyll)
Attachment #8519992 - Flags: review?(dteller)
Comment on attachment 8519992 [details] [diff] [review]
bug1061521.diff

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

This looks good, but I'm not sure what you mean by `the "pasted text" error`.

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +90,5 @@
>  }
>  
>  function handleException(ex)
>  {
> +  let str = ex.message;

I'd rather have `let str = "" + ex`, just in case.

@@ +96,5 @@
>      // Argh, assertion failure within this file!  Give up.
>      throw ex;
>    } else {
>      // File or memory reporter problem.  Print a message.
> +    updateMainAndFooter(ex.message, HIDE_FOOTER, "badInputWarning");

Let's reuse `str`.

@@ +644,5 @@
>        },
>        onStopRequest: function(aR, aC, aStatusCode) {
>          try {
>            if (!Components.isSuccessCode(aStatusCode)) {
> +            throw Components.Exception(aStatusCode);

First argument to `Components.Exception` should be the message.
Attachment #8519992 - Flags: review?(dteller)
Hi Jeff, any news about this bug?
Flags: needinfo?(jeffgodwyll)
Hello Yoric,
I am returning to bugzilla after a long time. So thinking of an easy bug now. 

Going through all the previous comments, I see that this bug is almost near to it completion. Also Jeff hasnt replied since 1.5 months. So, shall i start working afresh on this or let us wait for Jeff ?
Attached patch Fixed "throw" in aboutMemory.js (obsolete) — Splinter Review
Fixed the "throw" problems in aboutMemory.js as requested in Comment 0 of the bug.
Assignee: nobody → allamsetty.anup
Attachment #8484745 - Attachment is obsolete: true
Attachment #8486401 - Attachment is obsolete: true
Attachment #8519992 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Flags: needinfo?(jeffgodwyll)
Attachment #8693195 - Flags: review?(dteller)
Comment on attachment 8693195 [details] [diff] [review]
Fixed "throw" in aboutMemory.js

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

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +90,5 @@
>  }
>  
>  function handleException(ex)
>  {
> +  let str = ex.message;

As I mentioned in a previous review, let's make this `let str = "" + ex`.

@@ +98,3 @@
>    } else {
>      // File or memory reporter problem.  Print a message.
> +    updateMainAndFooter(ex.message, HIDE_FOOTER, "badInputWarning");

Use `str` instead of `ex.message`.

@@ +660,5 @@
>        onStopRequest: function(aR, aC, aStatusCode) {
>          try {
>            if (!Components.isSuccessCode(aStatusCode)) {
> +            throw new Components.Exception("Error while reading gzip file: " +
> +                                           aStatusCode);

If you look at the documentation of Components.Exception, you'll see that it's `Components.Exception(message, result)`
Attachment #8693195 - Flags: review?(dteller) → feedback+
Attached patch Fixed "throw" in aboutMemory.js (obsolete) — Splinter Review
Made the changes as requested in comment 34.
Attachment #8693195 - Attachment is obsolete: true
Attachment #8699843 - Flags: review?(dteller)
Comment on attachment 8699843 [details] [diff] [review]
Fixed "throw" in aboutMemory.js

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

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +91,5 @@
>  }
>  
>  function handleException(ex)
>  {
> +  `let str = "" + ex`;

Without the backticks, please. Otherwise, it won't define `str`.
Attachment #8699843 - Flags: review?(dteller) → feedback+
Removed the backticks as suggested in the above comment.
Attachment #8699843 - Attachment is obsolete: true
Attachment #8700066 - Flags: review?(dteller)
Comment on attachment 8700066 [details] [diff] [review]
Fixed "throw" in aboutMemory.js

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

Looks good to me.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eced1f258029

Note that I'll be away for two weeks, so you may need to find someone else to help you with landing.
Attachment #8700066 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach-Teller [:Yoric] (I will return in 2016 - use "needinfo") from comment #38)
> Comment on attachment 8700066 [details] [diff] [review]
> Fixed "throw" in aboutMemory.js
> 
> Review of attachment 8700066 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eced1f258029
> 
> Note that I'll be away for two weeks, so you may need to find someone else
> to help you with landing.

The patch got busted in one mochitest and failed in 9 other mochitests. So, looks like the patch is not yet fixed completely.
Are you still working on this, Anupkumar?
Flags: needinfo?(allamsetty.anup)
Unassigning the bug (won't be able to contribute in a while.)
Assignee: allamsetty.anup → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(allamsetty.anup)
Hey Rutuja, would you like to try finishing this patch off? It looks like a good chunk of the work is done, but there are apparently some failing tests due to it that we need to investigate.
Flags: needinfo?(rutuja.r.surve)
Sure, shall try doing it..
Flags: needinfo?(rutuja.r.surve)
Great! Yoric, is it okay if I mentor this one for Rutuja?
Flags: needinfo?(dteller)
While I'm waiting to hear back from Yoric, Rutuja, feel free to ask me any questions about how to approach this one, how to apply the patch, or how to run the tests that are apparently failing with the patch.
Hey Mike, I tried the command as in comment 24,I'm getting this as the result:
TEST-PASS | leakcheck | default process: no leaks detected!
runtests.py | Running tests: end.
0 INFO TEST-START | Shutdown
1 INFO Passed:  7
2 INFO Failed:  0
3 INFO Todo:    0
4 INFO SimpleTest FINISHED
SUITE-END | took 121s
How do I locate the failing testcases?
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #40)
> Are you still working on this, Anupkumar?

Hello Mike, 

Just updating details regarding the patch I've submitted:

The patch is failing in the following two tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eced1f258029&selectedJob=14742948

I think, I have to modify the code of these tests according to the patch.
Assignee: nobody → rutuja.r.surve
(In reply to Anup Kumar [:Anupkumar] from comment #47)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #40)
> > Are you still working on this, Anupkumar?
> 
> Hello Mike, 
> 
> Just updating details regarding the patch I've submitted:
> 
> The patch is failing in the following two tests:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=eced1f258029&selectedJob=14742948
> 
> I think, I have to modify the code of these tests according to the patch.

Hey Anup, is it alright if Rutuja try to fix those tests?
Flags: needinfo?(allamsetty.anup)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #48)
> (In reply to Anup Kumar [:Anupkumar] from comment #47)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #40)
> > > Are you still working on this, Anupkumar?
> > 
> > Hello Mike, 
> > 
> > Just updating details regarding the patch I've submitted:
> > 
> > The patch is failing in the following two tests:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=eced1f258029&selectedJob=14742948
> > 
> > I think, I have to modify the code of these tests according to the patch.
> 
> Hey Anup, is it alright if Rutuja try to fix those tests?

Sure, no worries. I'm trying for GSoC currently and I think, it would be a good start for Rutuja with this bug.
Flags: needinfo?(allamsetty.anup)
The exception we are facing currently is the same as in Bug 701256 - Intermittent test_aboutmemory.xul | Timed out while polling clipboard for pasted data. and | pasted text doesn't match for amFrame. Hence, a comparison of the content of the clipboard text and pasted text needs to be made to solve this issue.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #44)
> Great! Yoric, is it okay if I mentor this one for Rutuja?

Of course, be my guest!
Flags: needinfo?(dteller)
Hi! with log statements like below, I'm trying to test if the clipboard content (actual) and the expected content (aExpected) matches or not. I'm not being able to trace the expected and the clipboard text  despite of including them in the dump statements. Any idea as to how I should proceed? 
            ok(false, "pasted text doesn't match");
            dump("******EXPECTED******\n");
            dump(aExpected);
            dump("*******ACTUAL*******\n");
            dump(actual);
            dump("********************\n");
Interesting. Is anything being dumped out? The "******EXPECTED******" string, for example?
Flags: needinfo?(rutuja.r.surve)
Nothing is getting dumped out, both the "actual" and the "expected" string
Flags: needinfo?(rutuja.r.surve)
(In reply to Rutuja from comment #54)
> Nothing is getting dumped out, both the "actual" and the "expected" string

Interesting. If you do dumping earlier in the test, does it work? Does dump work anywhere?

If it does, and you're not seeing it here, are you certain you're entering the block of code that is supposed to be dumping the strings?
Mentor: dteller → mconley
Flags: needinfo?(rutuja.r.surve)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #55)
> (In reply to Rutuja from comment #54)
> > Nothing is getting dumped out, both the "actual" and the "expected" string
> 
> Interesting. If you do dumping earlier in the test, does it work? Does dump
> work anywhere?
> 
> If it does, and you're not seeing it here, are you certain you're entering
> the block of code that is supposed to be dumping the strings?

Yes, dump does work if it is done earlier in the test. In fact, I do get "Clipboard has the expected contents" for INFO TESTS 3,4 and 5 (they are passing). However, with test 6 and 16, I'm getting: INFO TEST UNEXPECTED-FAIL: "pasted text doesn't match". When I try to dump the expexted and the actual clipboard text along with a dummy dump line, the dummy line gets printed along but instead of the clipboard and expected values, I'm getting: "Invalid memory report(s): missing 'hasMozMallocUsableSize' propertyError". I have attached the complete trace here. Since the test does pass in some cases, I think the block of code that's supposed to be dumping the strings is fine.
Flags: needinfo?(rutuja.r.surve)
(In reply to Rutuja from comment #56)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #55)
> > (In reply to Rutuja from comment #54)
> > > Nothing is getting dumped out, both the "actual" and the "expected" string
> > 
> > Interesting. If you do dumping earlier in the test, does it work? Does dump
> > work anywhere?
> > 
> > If it does, and you're not seeing it here, are you certain you're entering
> > the block of code that is supposed to be dumping the strings?
> 
> Yes, dump does work if it is done earlier in the test. In fact, I do get
> "Clipboard has the expected contents" for INFO TESTS 3,4 and 5 (they are
> passing). However, with test 6 and 16, I'm getting: INFO TEST
> UNEXPECTED-FAIL: "pasted text doesn't match". When I try to dump the
> expexted and the actual clipboard text along with a dummy dump line, the
> dummy line gets printed along but instead of the clipboard and expected
> values, I'm getting: "Invalid memory report(s): missing
> 'hasMozMallocUsableSize' propertyError". I have attached the complete trace
> here. Since the test does pass in some cases, I think the block of code
> that's supposed to be dumping the strings is fine.
Attached file Trace file the copyandpaste check test (obsolete) —
(In reply to Rutuja from comment #57)
> (In reply to Rutuja from comment #56)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #55)
> > > (In reply to Rutuja from comment #54)
> > > > Nothing is getting dumped out, both the "actual" and the "expected" string
> > > 
> > > Interesting. If you do dumping earlier in the test, does it work? Does dump
> > > work anywhere?
> > > 
> > > If it does, and you're not seeing it here, are you certain you're entering
> > > the block of code that is supposed to be dumping the strings?
> > 
> > Yes, dump does work if it is done earlier in the test. In fact, I do get
> > "Clipboard has the expected contents" for INFO TESTS 3,4 and 5 (they are
> > passing). However, with test 6 and 16, I'm getting: INFO TEST
> > UNEXPECTED-FAIL: "pasted text doesn't match". When I try to dump the
> > expexted and the actual clipboard text along with a dummy dump line ("hello! do u work, dump??"), the
> > dummy line gets printed, but instead of the clipboard and expected
> > values, I get: "Invalid memory report(s): missing
> > 'hasMozMallocUsableSize' propertyError". I have attached the complete trace
> > here. Since the test does pass in some cases, I think the block of code
> > that's supposed to be dumping the strings is fine.
Comment on attachment 8734047 [details]
Trace file the copyandpaste check test

Ah, yeah, I see the dumps at the end there:

******EXPECTED******
Invalid memory report(s): missing 'hasMozMallocUsableSize' property*******ACTUAL*******
Error: Invalid memory report(s): missing 'hasMozMallocUsableSize' property********************
> ******EXPECTED******
> Invalid memory report(s): missing 'hasMozMallocUsableSize'
> property*******ACTUAL*******
> Error: Invalid memory report(s): missing 'hasMozMallocUsableSize'
> property********************

It should be fine to add "Error: " to the start of the |expectedBad| string.
Where do I find the contents of the various INFO TESTS and what input are they checking? Since only a few of the info tests show that the clipboard content matches the expected one, but the same code is used for copy-paste, either all test cases must pass or all of them must fail with respect to copy-paste. Since there is partial passing, the result seems to be changing with respect to loading of different input to the frame.
Flags: needinfo?(mconley)
Hey Rutuja - sorry for the delay; it was a long weekend holiday in Canada.

(In reply to Rutuja from comment #62)
> Where do I find the contents of the various INFO TESTS and what input are
> they checking?

Note that the "INFO" in "INFO TESTS" is the test harness alerting out information about one of the tests, so a line like this:

1 INFO TEST-START | toolkit/components/aboutmemory/tests/test_aboutmemory3.xul

Is saying:

"Information: The test at toolkit/components/aboutmemory/tests/test_aboutmemory3.xul has started".

and

3 INFO TEST-PASS | toolkit/components/aboutmemory/tests/test_aboutmemory3.xul | Clipboard has the expected contents 

means

"Information: An assertion inside toolkit/components/aboutmemory/tests/test_aboutmemory3.xul has passed. The message to go with the assertion was 'Clipboard has the expected contents'".

> Since only a few of the info tests show that the clipboard
> content matches the expected one, but the same code is used for copy-paste,
> either all test cases must pass or all of them must fail with respect to
> copy-paste.

Not necessarily true. This test is looping over a series of about:memory logs, loading them, and doing the copy-to-clipboard. It's possible that some of those still work as they did before the patch in this bug, but that some of them have changed. I'm guessing the ones that are likely to have changed are ones that have errors in them, since this is what the patch in this bug deals with.

Here's the list of inputs / expected outputs that are being fed through the test: https://hg.mozilla.org/mozilla-central/file/d1d47ba19ce9/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul#l486

What I'd suggest is trying to remove noise from this problem by trying to determine which ones of these pass, and which ones fail, and then focus solely on the failing ones.

I know the test is laid out in kind of a strange way, but I'll happily guide you through it - just let me know if there are any parts that need illumination.

> Since there is partial passing, the result seems to be changing
> with respect to loading of different input to the frame.

Yes, that's expected. My hypothesis is that we've changed how the errors are being rendered for one of the inputs - likely memory-reports-bad.json - and that the expected output[1] either needs to be updated, or the way that the output is being rendered from the new exceptions needs to be corrected.

[1]: https://hg.mozilla.org/mozilla-central/file/d1d47ba19ce9/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul#l278
Flags: needinfo?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #63)
> Hey Rutuja - sorry for the delay; it was a long weekend holiday in Canada.
> 
> (In reply to Rutuja from comment #62)
> > Where do I find the contents of the various INFO TESTS and what input are
> > they checking?
> 
> Note that the "INFO" in "INFO TESTS" is the test harness alerting out
> information about one of the tests, so a line like this:
> 
> 1 INFO TEST-START |
> toolkit/components/aboutmemory/tests/test_aboutmemory3.xul
> 
> Is saying:
> 
> "Information: The test at
> toolkit/components/aboutmemory/tests/test_aboutmemory3.xul has started".
> 
> and
> 
> 3 INFO TEST-PASS |
> toolkit/components/aboutmemory/tests/test_aboutmemory3.xul | Clipboard has
> the expected contents 
> 
> means
> 
> "Information: An assertion inside
> toolkit/components/aboutmemory/tests/test_aboutmemory3.xul has passed. The
> message to go with the assertion was 'Clipboard has the expected contents'".
> 
> > Since only a few of the info tests show that the clipboard
> > content matches the expected one, but the same code is used for copy-paste,
> > either all test cases must pass or all of them must fail with respect to
> > copy-paste.
> 
> Not necessarily true. This test is looping over a series of about:memory
> logs, loading them, and doing the copy-to-clipboard. It's possible that some
> of those still work as they did before the patch in this bug, but that some
> of them have changed. I'm guessing the ones that are likely to have changed
> are ones that have errors in them, since this is what the patch in this bug
> deals with.
> 
> Here's the list of inputs / expected outputs that are being fed through the
> test:
> https://hg.mozilla.org/mozilla-central/file/d1d47ba19ce9/toolkit/components/
> aboutmemory/tests/test_aboutmemory3.xul#l486
> 
> What I'd suggest is trying to remove noise from this problem by trying to
> determine which ones of these pass, and which ones fail, and then focus
> solely on the failing ones.
> 
> I know the test is laid out in kind of a strange way, but I'll happily guide
> you through it - just let me know if there are any parts that need
> illumination.
> 
> > Since there is partial passing, the result seems to be changing
> > with respect to loading of different input to the frame.
> 
> Yes, that's expected. My hypothesis is that we've changed how the errors are
> being rendered for one of the inputs - likely memory-reports-bad.json - and
> that the expected output[1] either needs to be updated, or the way that the
> output is being rendered from the new exceptions needs to be corrected.
> 
> [1]:
> https://hg.mozilla.org/mozilla-central/file/d1d47ba19ce9/toolkit/components/
> aboutmemory/tests/test_aboutmemory3.xul#l278

Yes sure, I'll try targeting the failing tests and let you know in case I have doubts.
Any updates on this Rutuja?
Flags: needinfo?(rutuja.r.surve)
Attached file test_aboutmemory3.xul
This is the updated test.xul file. All test cases pass for this one
Flags: needinfo?(rutuja.r.surve)
I get this as the final result on the console with above attached modified xul file:
TEST-PASS | leakcheck | default process: no leaks detected!
runtests.py | Running tests: end.
0 INFO TEST-START | Shutdown
1 INFO Passed:  7
2 INFO Failed:  0
3 INFO Todo:    0
4 INFO SimpleTest FINISHED
SUITE-END | took 56s
Attached file Txt format of the modified xul file (obsolete) —
Great, thank you Rutuja!

So instead of posting the changed file in its entirety, what we'll need you to post is a patch, which shows the differences you've made to the file.

Here are some instructions on creating a patch. Needinfo me if you need any help with any of the steps:

https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #69)
> Great, thank you Rutuja!
> 
> So instead of posting the changed file in its entirety, what we'll need you
> to post is a patch, which shows the differences you've made to the file.
> 
> Here are some instructions on creating a patch. Needinfo me if you need any
> help with any of the steps:
> 
> https://developer.mozilla.org/en-US/docs/Mercurial/
> Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F

Yes sure..
(In reply to Rutuja from comment #71)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #69)
> > Great, thank you Rutuja!
> > 
> > So instead of posting the changed file in its entirety, what we'll need you
> > to post is a patch, which shows the differences you've made to the file.
> > 
> > Here are some instructions on creating a patch. Needinfo me if you need any
> > help with any of the steps:
> > 
> > https://developer.mozilla.org/en-US/docs/Mercurial/
> > Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-
> > in_for_me.3F
> 
> Yes sure..

Please check the "Change timeout interval and add error prefix to the expected string code for error msgs" patch only. I uploaded the previous patch "Changes with respect to prefix made" by mistake.
Comment on attachment 8737619 [details] [diff] [review]
Changes with respect to prefix made

You can mark a patch (or any attachment) as "obsolete". Click on the "Details" link for the patch, then click on the "edit details" link, then select the "obsolete" check box, then click on "Submit". (This is not obvious! :)
Attachment #8737619 - Attachment is obsolete: true
(In reply to Nicholas Nethercote [:njn] from comment #74)
> Comment on attachment 8737619 [details] [diff] [review]
> Changes with respect to prefix made
> 
> You can mark a patch (or any attachment) as "obsolete". Click on the
> "Details" link for the patch, then click on the "edit details" link, then
> select the "obsolete" check box, then click on "Submit". (This is not
> obvious! :)

Oh.. I didn't know that.. Thanks! :)
Comment on attachment 8737626 [details] [diff] [review]
Change timeout interval and add error prefix to the expected string code for error msgs

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

Thanks for the patch Rutuja! I have some questions - please see below.

Also, next time your patch gets posted, you can add it to my review queue by setting the "review" flag on the attachment to ?, and then putting my email address, mconley@mozilla.com, in the text box that shows after. You can change the flags on an attachment by clicking "Details" next to it on the Bugzilla page.

::: toolkit/components/aboutmemory/tests/test_aboutmemory3.xul
@@ +110,5 @@
>            SimpleTest.ok(true, "Clipboard has the expected contents");
>            aNext();
>          } else {
> +           if(numFailures === 1){
> +            aExpected="Error: " + aExpected;

As discussed in IRC, what I think we should do here instead is to update the expected strings to reflect the usage of the new Error objects.

For example, this line:

http://searchfox.org/mozilla-central/source/toolkit/components/aboutmemory/tests/test_aboutmemory3.xul#278

I think this is more robust and explicit than adding additional logic to the code that does the comparison.

@@ +122,5 @@
>              dump(actual);
>              dump("********************\n");
>              finish();
>            } else {
> +            setTimeout(copyPasteAndCheck, 500);

I'm curious - why is this necessary?
Attachment #8737626 - Flags: review-
Attachment #8738037 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #76)
> Comment on attachment 8737626 [details] [diff] [review]
> Change timeout interval and add error prefix to the expected string code for
> error msgs
> 
> Review of attachment 8737626 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch Rutuja! I have some questions - please see below.
> 
> Also, next time your patch gets posted, you can add it to my review queue by
> setting the "review" flag on the attachment to ?, and then putting my email
> address, mconley@mozilla.com, in the text box that shows after. You can
> change the flags on an attachment by clicking "Details" next to it on the
> Bugzilla page.
> 
> ::: toolkit/components/aboutmemory/tests/test_aboutmemory3.xul
> @@ +110,5 @@
> >            SimpleTest.ok(true, "Clipboard has the expected contents");
> >            aNext();
> >          } else {
> > +           if(numFailures === 1){
> > +            aExpected="Error: " + aExpected;
> 
> As discussed in IRC, what I think we should do here instead is to update the
> expected strings to reflect the usage of the new Error objects.
> 
> For example, this line:
> 
> http://searchfox.org/mozilla-central/source/toolkit/components/aboutmemory/
> tests/test_aboutmemory3.xul#278
> 
> I think this is more robust and explicit than adding additional logic to the
> code that does the comparison.
> 
> @@ +122,5 @@
> >              dump(actual);
> >              dump("********************\n");
> >              finish();
> >            } else {
> > +            setTimeout(copyPasteAndCheck, 500);
> 
> I'm curious - why is this necessary?

I have updated the expected string to reflect the usage of the new Error Objects, as you had suggested. I had increased the timeout interval initially from 100 to 500, as I was getting 6 successes and 1 failure with 100, making it 500 helped pass all test-cases. 
However, with no additional change in the logic (by just modifying the expected string in the new patch that I've just attached), all test-cases now pass with interval=100. Prefix is the only change in this patch now and all 7 test-cases pass.
Attachment #8737340 - Attachment is obsolete: true
Attachment #8734047 - Attachment is obsolete: true
Comment on attachment 8738037 [details] [diff] [review]
Added error prefix "Error: " to the expectedBad string for error messages

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

This looks good to me.

I'm going to push your patch to our Try server to make sure the test passes in automation, and then I think this will be good to land!

Thank you, Rutuja!
Attachment #8738037 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #79)
> Comment on attachment 8738037 [details] [diff] [review]
> Added error prefix "Error: " to the expectedBad string for error messages
> 
> Review of attachment 8738037 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me.
> 
> I'm going to push your patch to our Try server to make sure the test passes
> in automation, and then I think this will be good to land!
> 
> Thank you, Rutuja!

Yes sure. What can I do next, in the meanwhile?
(In reply to Rutuja from comment #81)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #79)
> > Comment on attachment 8738037 [details] [diff] [review]
> > Added error prefix "Error: " to the expectedBad string for error messages
> > 
> > Review of attachment 8738037 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good to me.
> > 
> > I'm going to push your patch to our Try server to make sure the test passes
> > in automation, and then I think this will be good to land!
> > 
> > Thank you, Rutuja!
> 
> Yes sure. What can I do next, in the meanwhile?

According to the try push, there's still more work to do here. One of the jobs came back orange (meaning there was a test failure), and the failure is reported as being:

2921 INFO TEST-UNEXPECTED-FAIL | toolkit/components/aboutmemory/tests/test_aboutmemory4.xul | pasted text doesn't match

So can you try to make that one work locally as well?
Flags: needinfo?(rutuja.r.surve)
Comment on attachment 8738037 [details] [diff] [review]
Added error prefix "Error: " to the expectedBad string for error messages

Thanks for the new patch! For future reference, don't forget to mark your old ones as obsolete when you create new ones.
Attachment #8738037 - Attachment is obsolete: true
Comment on attachment 8738227 [details] [diff] [review]
Added error prefix "Error: " to the expected string for error messages. (test_aboutmemory3.xul and test_aboutmemory4.xul)

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

Looks good. Pushing another build to try.
Attachment #8738227 - Flags: review?(mconley) → review+
Try looks good! Setting checkin-needed for both patches.
Keywords: checkin-needed
Hrm, did one of the patches in here not get checked in?
Flags: needinfo?(cbook)
Yeah, both needed to go in. I'll land it.
Flags: needinfo?(rutuja.r.surve)
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/41a29164d665
https://hg.mozilla.org/mozilla-central/rev/16ad4d244d7d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.