deleteTabValue should not throw an exception

RESOLVED FIXED in Firefox 6

Status

()

Firefox
Session Restore
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: ithinc, Unassigned)

Tracking

Trunk
Firefox 6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug] [mentor=zpao])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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");

Updated

6 years ago
Version: unspecified → 3.6 Branch
(Reporter)

Updated

6 years ago
OS: Windows XP → All
Hardware: x86 → All
Version: 3.6 Branch → Trunk
Seems reasonable...

This would be a good first bug for anybody looking to contribute.
Whiteboard: [good first bug]

Comment 2

6 years ago
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.
Whiteboard: [good first bug] → [good first bug] [mentor=jdm]
I know this code so sure, why not.
Whiteboard: [good first bug] [mentor=jdm] → [good first bug] [mentor=zpao]

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

6 years ago
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?
(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

6 years ago
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.
(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

6 years ago
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

6 years ago
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?
(Reporter)

Comment 10

6 years ago
Hi Alastair Robertson,

Thanks for your taking. Removing the else branch is enough for me.
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.
Attachment #532968 - Flags: feedback?(dietrich)
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.
Attachment #532968 - Flags: feedback?(dietrich) → feedback+

Comment 13

6 years ago
Created attachment 533616 [details] [diff] [review]
Patch 2

Added tests for deleting non-existent values to browser_350525.js
Attachment #532968 - Attachment is obsolete: true
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
Attachment #533616 - Flags: review+

Comment 15

6 years ago
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?
(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

6 years ago
(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.
http://hg.mozilla.org/mozilla-central/rev/2bdc3b130332
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6

Updated

5 years ago
Blocks: 732345
You need to log in before you can comment on or make changes to this bug.