700044 - Duplicate rule views after tab switching

Status: RESOLVED FIXED

(DevTools :: General, defect, P1)

Description

[Dave Camp (:dcamp)]

1) Open the highlighter, show the rule view
2) Open a new tab
3) Switch back to the first tab

The rule view now has two sets of rules.

Comment 1

[Dave Camp (:dcamp)]

We call toolShow() once from restoreToolState, and once from its button's click handler.  That's not ideal, but the tool should be robust against multiple show() calls.

Comment 2

[Dave Camp (:dcamp)]

Attachment: fix, no test

I can't think of a good way to test this.  Easy enough to trigger the bug (test can just call toolShow() twice in a row).  But I can't think offhand of a good way to make sure we don't get two load events, without just waiting a long time.

Comment 3

[Mihai Sucan [:msucan]]

(In reply to Dave Camp (:dcamp) from comment #2)
> Created attachment 572380 [details] [diff] [review] [diff] [details] [review]
> fix, no test
> 
> I can't think of a good way to test this.  Easy enough to trigger the bug
> (test can just call toolShow() twice in a row).  But I can't think offhand
> of a good way to make sure we don't get two load events, without just
> waiting a long time.

In such cases I usually write a test that does what the STR says, at the end I check the expected result.

Comment 4

[Dave Camp (:dcamp)]

(In reply to Mihai Sucan [:msucan] from comment #3)

> In such cases I usually write a test that does what the STR says, at the end
> I check the expected result.

The problem is deciding what "the end" is - I can't think of a good way to wait for "enough time that a second load event would have fired if things were broken" that would be racy.

Comment 5

[Rob Campbell [:rc] (:robcee)]

I've been using "traps" to detect multiple occurrences or things that shouldn't occur in my tests.

e.g.,
register one handler
when that one is received,
register another handler to your trap

* do something long-winded *
finish

if the trap is reached, dump a failure ok(false) and finish the test with your cleanup.

That seems to work well. If your "do something long-winded" test needs a bit more time, requestLongerTimeout(2) or 4 is usually enough.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Attachment: test

test fails without fix, passes with. I'm calling this a unittest.

Comment 7

[Dave Camp (:dcamp)]

Attachment: merged patch

Merged the two patches and rebased on top of my landing queue.  Set patch's user to rob, since clearly more novelty went into the test than the fix.  Still, would like msucan to review because I don't want to r+'ing my own code changes.

Comment 8

[Mihai Sucan [:msucan]]

Comment on attachment 572507 [details] [diff] [review]
merged patch

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

Patch looks good. The usual nits apply (PD boilerplate, arguments.callee).

Comment 9

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/f57c833bb23e

Comment 10

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/f57c833bb23e