Last Comment Bug 606681 - deleteTabValue should not throw an exception
: deleteTabValue should not throw an exception
Status: RESOLVED FIXED
[good first bug] [mentor=zpao]
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks: 732345
  Show dependency treegraph
 
Reported: 2010-10-23 08:52 PDT by ithinc
Modified: 2012-03-02 01:03 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removed throw statements and fixed old test (2.66 KB, patch)
2011-05-17 08:35 PDT, Alastair Robertson
dietrich: feedback+
Details | Diff | Splinter Review
Patch 2 (4.38 KB, patch)
2011-05-19 05:56 PDT, Alastair Robertson
paul: review+
Details | Diff | Splinter Review

Description ithinc 2010-10-23 08:52:13 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-CN; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; zh-CN; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11 (.NET CLR 3.5.30729)

I see no reason for deleteTabValue API to throw an exception when the key does not exist. It needs to be fixed, although we can use setTabValue(aTab, aKey, "") instead.

Reproducible: Always

Steps to Reproduce:
var ss = Cc["@mozilla.org/browser/sessionstore;1"].getService(Ci.nsISessionStore);
ss.deleteTabValue(gBrowser.mCurrentTab, "unread");
ss.deleteTabValue(gBrowser.mCurrentTab, "unread");
Comment 1 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-04-14 10:37:01 PDT
Seems reasonable...

This would be a good first bug for anybody looking to contribute.
Comment 2 Josh Matthews [:jdm] 2011-04-14 14:37:31 PDT
Paul, if you're willing to act as a point person for anyone who wants to fix this bug, you can replace my name in the mentor field.
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-04-14 15:02:36 PDT
I know this code so sure, why not.
Comment 4 Alastair Robertson 2011-05-14 07:59:30 PDT
I'd like to take on this bug if that's ok. This would be my first bug, so I don't know my way around the codebase all that well yet.

Could someone confirm that I am looking at the right function here: https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1366
and that it is line 1381 that is the problem?

Also, how do I actually run the testcase supplied by ithinc to reproduce this bug?
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-14 14:15:26 PDT
(In reply to comment #4)
> I'd like to take on this bug if that's okThis would be my first bug, so I
> don't know my way around the codebase all that well yet.

That's definitely ok. This is a good bug to get your feet wet.

> Could someone confirm that I am looking at the right function here:
> https://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/src/nsSessionStore.js#1366
> and that it is line 1381 that is the problem?

Correct. While we're at it, would you like to fix deleteWindowValue so that we have matching behavior?

> Also, how do I actually run the testcase supplied by ithinc to reproduce
> this bug?

Ideally you would write a test that can be run automatically, like the other tests in https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser/ and then you can run it with the rest of the session restore tests.

I'm not sure if there's a test for deleteValue in there (I would expect there to be one), but creating a new file is ok too. You can model it off one of the existing tests (and add it to the makefile).

If you need anything else, just let me know (I'll check back on Monday). I'm happy to help.
Comment 6 Alastair Robertson 2011-05-15 15:11:06 PDT
What behaviour do we want instead of throwing an exception in deleteTabValue? Just do nothing if the key doesn't exist?

(In reply to comment #5)
> Correct. While we're at it, would you like to fix deleteWindowValue so that
> we have matching behavior?

Yes, of course.
Comment 7 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-16 08:45:37 PDT
(In reply to comment #6)
> What behaviour do we want instead of throwing an exception in
> deleteTabValue? Just do nothing if the key doesn't exist?

Yes. We'll treat it just like the success case (since it sort of is).
Comment 8 Alastair Robertson 2011-05-17 08:35:13 PDT
Created attachment 532968 [details] [diff] [review]
Removed throw statements and fixed old test

Here is my first attempt at a patch. I haven't added any new tests, but have removed some old ones which expected exceptions to be thrown.
Comment 9 Alastair Robertson 2011-05-17 08:38:00 PDT
Was ithinc suggesting we just have setTabValue(aTab, aKey, "") in the deleteTabValue function, instead of duplicating the code?

(In reply to comment #5)
> I'm not sure if there's a test for deleteValue in there (I would expect
> there to be one), but creating a new file is ok too.
There are already a few testcases that use deleteTabValue. Do I just pick any one of them to add on to?
Comment 10 ithinc 2011-05-17 09:22:17 PDT
Hi Alastair Robertson,

Thanks for your taking. Removing the else branch is enough for me.
Comment 11 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-18 10:30:32 PDT
Comment on attachment 532968 [details] [diff] [review]
Removed throw statements and fixed old test

Dietrich: I'm ok with getting rid of the throws here (I look at it like removeEventListener which is silent if you pass a listener which isn't listening). But I wanted to check with you since you reviewed the patch in bug 345898 which added them. I can see some small value if passed a window/tab that sessionstore isn't tracking but other than that it seems unnecessary.

Alastair: Removing those is the right first step. The next step would be making sure that calling delete*Value doesn't throw when called for nonexistant values. I think adding to browser_350525.js and just calling delete*Value again after each has been called once should be enough. I think we'll want to keep throwing in some cases, but let's hold off on that until Dietrich responds.
Comment 12 Dietrich Ayala (:dietrich) 2011-05-18 18:30:44 PDT
Comment on attachment 532968 [details] [diff] [review]
Removed throw statements and fixed old test

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

this is ok, f=me. it is a minor behavioral change, so not sure if it's worth marking this bug with the compat keyword.
Comment 13 Alastair Robertson 2011-05-19 05:56:34 PDT
Created attachment 533616 [details] [diff] [review]
Patch 2

Added tests for deleting non-existent values to browser_350525.js
Comment 14 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-19 15:18:23 PDT
Comment on attachment 533616 [details] [diff] [review]
Patch 2

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

We'll need to fix the conflicts when bug 658259 lands, but otherwise, it looks great. Thanks a lot Alastair!

I pushed to try server just to make sure and it looks good: http://tbpl.mozilla.org/?tree=Try&rev=cc2cb6af2cac
Comment 15 Alastair Robertson 2011-05-19 15:38:24 PDT
Thanks for all your help with this bug Paul :)

Does solving the conflicts involve creating a new patch, or can it be done with what has already been submitted?
Comment 16 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-05-19 22:39:48 PDT
(In reply to comment #15)
> Thanks for all your help with this bug Paul :)

No problem. There are always more if you want to keep going ;) (seriously, I have a pile of session restore bugs ripe for the fixing)

> Does solving the conflicts involve creating a new patch, or can it be done
> with what has already been submitted?

Mercurial might handle it ok - it's just some changes in context. If it doesn't, it'll be a small conflict, so I can fix it when I check it in.

Speaking of checking in, since your patch doesn't have author information in there, is the name and email address you're using for bugmail ok? It would look like http://hg.mozilla.org/try/rev/a76a300e882b
Comment 17 Alastair Robertson 2011-05-20 06:15:11 PDT
(In reply to comment #16)
> No problem. There are always more if you want to keep going ;) (seriously, I
> have a pile of session restore bugs ripe for the fixing)
I'd like to contribute more in the future, so I'll have a look through bugzilla later (unless there were any specific bugs you had in mind).

> Speaking of checking in, since your patch doesn't have author information in
> there, is the name and email address you're using for bugmail ok? It would
> look like http://hg.mozilla.org/try/rev/a76a300e882b
Yes, that information is fine.
Comment 18 Matt Brubeck (:mbrubeck) 2011-05-20 10:05:20 PDT
http://hg.mozilla.org/mozilla-central/rev/2bdc3b130332

Note You need to log in before you can comment on or make changes to this bug.