772360 - Mozmill test failure //testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/ with error "The bookmark was created "

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Maniac Vlad Florin (:vladmaniac)]

Mozmill version: 1.5.13 
OS: Windows NT 5.1.2600 Service Pack 3 (x86)
Firefox version: Firefox default branch 
Test location: http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar
Error: TimeoutError@resource://mozmill/modules/utils.js:447 waitFor@resource://mozmill/modules/utils.js:485 @resource://mozmill/modules/controller.js:685 @resource://mozmill/modules/frame.js -> file:///c:/docume~1/mozilla/locals~1/temp/tmp11tgz7.mozmill-tests/tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js:47 endurance_run@resource://mozmill/stdlib/securable-module.js -> file:///c:/docume~1/mozilla/locals~1/temp/tmp11tgz7.mozmill-tests/lib/endurance.js:83 testAddRemoveBookmarkViaAwesomeBar@resource://mozmill/modules/frame.js -> file:///c:/docume~1/mozilla/locals~1/temp/tmp11tgz7.mozmill-tests/tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js:37 @resource://mozmill/modules/frame.js:563 @resource://mozmill/modules/frame.js:633 @resource://mozmill

Also other error caught locally while running on a win XP VM and reported to mozmill-crowd dashboard: "controller.waitForPageLoad(): Timeout waiting for page loaded. "

Crowd error report link: http://mozmill-crowd.blargon7.com/#/endurance/report/99ab6c49d005522227c5717d8a07015d
Ci error report link: mozmill-ci.blargon7.com/#/endurance/report/99ab6c49d005522227c5717d8a029143

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

So this is intermittent 

Passing result http://mozmill-crowd.blargon7.com/#/endurance/report/99ab6c49d005522227c5717d8a073bbe

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Which page are we trying to bookmark?

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Which page are we trying to bookmark?

This one 
http://mozqa.com/data/firefox/layout/mozilla_contribute.html
from the local test folder

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So the only thing I could imagine here is that we failed to click the star icon. It could be related to the waitForPageLoad() issue you was facing too. Is the latter one reproducible for you? I really wonder why that happens.

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> So the only thing I could imagine here is that we failed to click the star
> icon. It could be related to the waitForPageLoad() issue you was facing too.
> Is the latter one reproducible for you? I really wonder why that happens.

The failure on CI I cannot reproduce. I am currently running tests on the win XP VM to see if we can catch something again. Otherwise, I will put this in the backlog until we have some more results from the test machines in MV. 

I will follow up with my local results at the end of the day and we'll probably have a verdict with it.

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

I see no more failures locally so this is likely to appear sometimes when the system is heavily loaded 

http://mozmill-crowd.blargon7.com/#/endurance/reports?branch=All&platform=All&from=2012-07-10&to=2012-07-10

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This has failed again, however this time on aurora, and fr locale. Same OS. http://mozmill-ci.blargon7.com/#/endurance/report/99ab6c49d005522227c5717d8a12a079

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #7)
> This has failed again, however this time on aurora, and fr locale. Same OS.
> http://mozmill-ci.blargon7.com/#/endurance/report/
> 99ab6c49d005522227c5717d8a12a079

My windows xp VM is running tests from yesterday and reports to mozmill-crowd. no failures so far. I need to load the system maybe I can see it, in that case would be a simple fix

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

I cannot see this error in 7 days of reports 

http://mozmill-ci.blargon7.com/#/endurance/reports?branch=All&platform=All&from=2012-08-01&to=2012-08-07

Can we close this bug for now?

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

Closing as WFM considering the 7 day report from comment 9 and the fact I got no feedback on this one. Anyone please feel free to reopen if this occurs again and you are spotting it on any of the dashboards.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This failed today for 15 beta builds:

http://mozmill-ci.blargon7.com/#/endurance/report/d87d47fd1034f072b9bece6ee6830364

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> This failed today for 15 beta builds:
> 
> http://mozmill-ci.blargon7.com/#/endurance/report/
> d87d47fd1034f072b9bece6ee6830364

Will check. Thanks for reporting!

Comment 13

[Maniac Vlad Florin (:vladmaniac)]

Unfortunately I can't see this in my testing box 
http://mozmill-crowd.blargon7.com/#/endurance/report/d87d47fd1034f072b9bece6ee6872bbe

When looking at the code, we wait 5s (default timeout value) for the bookmark to be created, which is more than enough, and we test the bookmark creation using the backend API (bookmarksService) which is reliable. Code snippet is: 

controller.waitFor(function () {
      return places.bookmarksService.isBookmarked(URI);
    }, "The bookmark was created");

So my guess is that we do not click the star properly. In this case I would suggest replacing controller.click with controller.mouseDown and controller.mouseUp

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

In which iteration does it throw the failure? I don't think replacing click() will solve the problem at all.

Comment 15

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> In which iteration does it throw the failure? I don't think replacing
> click() will solve the problem at all.

Could not say cause I'm not seeing this locally.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

You have the test report and the jenkins console output to check that.

Comment 17

[Maniac Vlad Florin (:vladmaniac)]

I miss any checkpoints entries in the report so the conclusion is that is fails in the first iterations. 

The report is : http://mozauto.iriscouch.com/mozmill-ci/d87d47fd1034f072b9bece6ee6830364

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We might want to check that the star has been filled out. Given that this failure is rare you should try to rebuild such a broken run in the jenkins master and report to mozmill-crowd.

Comment 19

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> We might want to check that the star has been filled out. Given that this
> failure is rare you should try to rebuild such a broken run in the jenkins
> master and report to mozmill-crowd.

Green build http://10.250.73.243:8080/job/release-mozilla-beta_endurance/11/

Comment 20

[Maniac Vlad Florin (:vladmaniac)]

Happened again - http://mozmill-ci.blargon7.com/#/endurance/report/d87d47fd1034f072b9bece6ee69fdaf1

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #17)
> I miss any checkpoints entries in the report so the conclusion is that is
> fails in the first iterations. 
> 
> The report is :
> http://mozauto.iriscouch.com/mozmill-ci/d87d47fd1034f072b9bece6ee6830364

If an endurance test fails then the report is lost and therefore there is no way to know what the current iteration/entity is. This would be useful to know, so perhaps a --verbose option in the automation script that dumps this information to stdout would be useful when diagnosing this and similar issues?

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Dave Hunt (:davehunt) from comment #21)
> If an endurance test fails then the report is lost and therefore there is no
> way to know what the current iteration/entity is. This would be useful to

Huh, really? That should not happen. Where in the code we move the recorded data to the persisted object? Would a finalize method in teardownModule help?

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #22)
> (In reply to Dave Hunt (:davehunt) from comment #21)
> > If an endurance test fails then the report is lost and therefore there is no
> > way to know what the current iteration/entity is. This would be useful to
> 
> Huh, really? That should not happen. Where in the code we move the recorded
> data to the persisted object? Would a finalize method in teardownModule help?

It's always been the case. The results get appended after each test: http://hg.mozilla.org/qa/mozmill-tests/file/ce8193bf5289/lib/endurance.js#l89

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Dave Hunt (:davehunt) from comment #23)
> It's always been the case. The results get appended after each test:
> http://hg.mozilla.org/qa/mozmill-tests/file/ce8193bf5289/lib/endurance.js#l89

We should run it in a try/catch in this case to ensure we always push the partial information until this stage. It could help to identify this failure.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #24)
> We should run it in a try/catch in this case to ensure we always push the
> partial information until this stage. It could help to identify this failure.

Dave, what do you think? Would you mind doing that so that we have more information for this failure?

Comment 26

[Maniac Vlad Florin (:vladmaniac)]

Thanks Dave for the fix. 

So the latest failure report is this 
http://mozmill-ci.blargon7.com/#/endurance/report/671677a5d9d5ca25f3cf5ae1c4223bed

Given that Dave unblocked this yesterday I'll move on from here with the investigation. Lets see if we can get the data from the jenkins report now.

Comment 27

[Maniac Vlad Florin (:vladmaniac)]

So the plan is to 
* rebuild the failing run in Jenkins and report to mozmill-crowd
  [done] and reproduced the failure, but its intermittent 
  http://10.250.73.243:8080/job/release-mozilla-release_endurance/22/

* run the build on my testing box under the win xp sp3 virtual machine
  [in progress] so far no errors for Nightly, switching to release branch next
  http://mozmill-crowd.blargon7.com/#/endurance/report/671677a5d9d5ca25f3cf5ae1c422b0c2

Comment 28

[Maniac Vlad Florin (:vladmaniac)]

The error reproduced locally in my testing box right now, failed in the first iteration. 
The report is here: 

http://mozmill-crowd.blargon7.com/#/endurance/report/671677a5d9d5ca25f3cf5ae1c4231c56
Reproducing does not depend on running the whole testrun, since I ran only this test from the endurance test run.

Comment 29

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix patch v1.0

After investigating locally I can see that we do not click on the star button at the right moment, at least sometimes, therefore we get the error message because the bookmark is not created. 

The fix patch replaces controller.click(starButton) with controller.waitThenClick(starButton) and then adds a expect call to see if the star button UI has changed after we click on this element. This fixes the problem at least for me, I cannot reproduce the failure locally anymore and mozmill-crowd is full of reports to prove that.

Comment 30

[Maniac Vlad Florin (:vladmaniac)]

The extra expect call does not harm the test, on the contrary it help to trigger the right error if the test will fail again.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #29)
> After investigating locally I can see that we do not click on the star
> button at the right moment, at least sometimes, therefore we get the error
> message because the bookmark is not created. 

Why we don't click the star button? Why we have to use a wait method before clicking on it? This node is always present and not hidden. So I don't understand your statement.

Comment 32

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #31)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #29)
> > After investigating locally I can see that we do not click on the star
> > button at the right moment, at least sometimes, therefore we get the error
> > message because the bookmark is not created. 
> 
> Why we don't click the star button? Why we have to use a wait method before
> clicking on it? This node is always present and not hidden. So I don't
> understand your statement.

I cannot tell you why we are not clicking it. But we don't. The test just freezes and the star button does not change UI state to 'starred' (yellow).

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #32)
> > Why we don't click the star button? Why we have to use a wait method before
> > clicking on it? This node is always present and not hidden. So I don't
> > understand your statement.
> 
> I cannot tell you why we are not clicking it. But we don't. The test just
> freezes and the star button does not change UI state to 'starred' (yellow).

So I would like that you investigate that a bit more, e.g. after how many milliseconds the test is not failing when clicking on the icon.

Comment 34

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 658838 [details] [diff] [review]
fix patch v1.0

Given my last comment you get a feedback+ because the code is correct and probably working as you say. But I wanna be sure why that happens and if this is the right fix or if there is a bug in Firefox.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please come up with an analysis on this ASAP. It's failing too often and I would like to get this failure fixed.

Comment 36

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #35)
> Please come up with an analysis on this ASAP. It's failing too often and I
> would like to get this failure fixed.

We have to add a waitFor to wait for the star button change [1], meaning the 'starred' attribute which is inserted after we click the star. After the patch has passed internal reviews it will be uploaded asap

mxr reference [1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug432599.js#63

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's a way better analysis as the one in comment 32. Sounds totally reasonable. Thanks Vlad!

Comment 38

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix patch v1.1

* I am going to obsolete v1.0 since the real fix is contained in patch v1.1
* Passed internal r
* Tested on the win xp VM and Linux Ubuntu

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 660353 [details] [diff] [review]
fix patch v1.1

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

::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +43,5 @@
>  
> +    controller.waitFor(function () {
> +      return starButton.getNode().hasAttribute("starred");
> +    }, "Star button state changed");
> +

hm, while checking it again a question arised. Why do we have to wait twice? Once for the button to be filled and then for places.bookmarksService.isBookmarked(URI). Shouldn't the last waitFor be enough?

Comment 40

[Maniac Vlad Florin (:vladmaniac)]

nope because the last waitFor waiting for polling the bookmarkService to check that the bookmark was created. 
the first waitFor is strictly checking for the UI of the star. 

The flow is: 
click star - wait for UI - once UI finished we call the insertBookmark method from nsIBookmarksService interface which also needs a small timeout to finish the routine.

Please check mxr for reference.

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Well, again I don't see a reason then why we would have to do a waitFor() for isBookmarked(). If the starred attribute is the important one, which I now think, we could completely get rid of the isBookmarked() call.

Comment 42

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 660353 [details] [diff] [review]
fix patch v1.1

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

::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +43,5 @@
>  
> +    controller.waitFor(function () {
> +      return starButton.getNode().hasAttribute("starred");
> +    }, "Star button state changed");
> +

I understand that we might want to wait for the star button to have this attribute (and agree with Henrik that this probably means we can drop the following waitFor), but I don't see how this would fix the issue.

You mentioned in comment 29 that we do not click the star button at the right moment, and therefore the bookmark is not created. In the associated patch you used waitThenClick to address this. Why would this no longer be needed? If the star button is not clicked correctly then I would equally expect that neither the starred attribute would be set or the bookmark be added.

Comment 43

[Maniac Vlad Florin (:vladmaniac)]

After studying some code in mxr I can say for sure that this is the cause now. I've explained the flow in comment 40. The waitThenClick is not necessary at all. Its the waitFor UI change we want here. After this is finished, the bookmark is really added

Comment 44

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This still doesn't make sense to me. If after clicking we want an end result of isBookmarked to return true, then it shouldn't matter what happens regarding the UI between the click and isBookmarked.

Comment 45

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix patch v1.2

Sorry for the nits. They are fixed now

Comment 46

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 660393 [details] [diff] [review]
fix patch v1.2

Ups, wrong bug

Comment 47

[Maniac Vlad Florin (:vladmaniac)]

So after a good chat with Dave on irc we are unfortunately not ready yet here.

Comment 48

[Maniac Vlad Florin (:vladmaniac)]

I think we are doing the flow wrongly. This link [1] shows that the flow would be: 

* click the star
* assert for the bookmark to be created
* wait for the UI change of the star

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_bug581253.js#30

Comment 49

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The star is not always visible, e.g. for about:blank. I know we load the page before in setupModule but probably that's the problem here. I would suggest that we add a waitFor until the star is visible and then click on it. This method is different from waitThenClick(). With this method we will at least know if we are trying to click too early.

Comment 50

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #49)
> The star is not always visible, e.g. for about:blank. I know we load the
> page before in setupModule but probably that's the problem here. I would
> suggest that we add a waitFor until the star is visible and then click on
> it. This method is different from waitThenClick(). With this method we will
> at least know if we are trying to click too early.

Sounds reasonable. I will continue with implementing this solution and of course testing it

Comment 51

[Maniac Vlad Florin (:vladmaniac)]

Attachment: fix patch v1.3

* Implemented Henrik's suggestion
* the 'hidden' property is false when the star is visible and 'true' when the star is hidden, so waiting for that
* I tested and I cannot see the any errors.

Comment 52

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 660800 [details] [diff] [review]
fix patch v1.3

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

::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +40,5 @@
>      var URI = utils.createURI(LOCAL_TEST_PAGE);
>  
> +    // Check that star button is visible
> +    controller.waitFor(function () {
> +      return starButton.getNode().hidden == false;

As mentioned during today's ask an expert session, please use utils.isDisplayed for this. If this does not currently check the hidden property then we should enhance it to do so.

Comment 53

[Maniac Vlad Florin (:vladmaniac)]

Even if we enhance utils.isDisplayed to check for the hidden property, even if the hidden property has a bad value, the test will still pass. We need to re-think this properly in other bug which should probably be in another sprint. 

Please consider approving the waitFor solution for the moment, the effect is similar.

Comment 54

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #53)
> Even if we enhance utils.isDisplayed to check for the hidden property, even
> if the hidden property has a bad value, the test will still pass. We need to
> re-think this properly in other bug which should probably be in another
> sprint. 

Why would the test still pass? I don't understand why your check for visibility is good enough for within the test but not as part of isDisplayed. What other bug are you referring to here?

Comment 55

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.4

* requested method implemented

Comment 56

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 661226 [details] [diff] [review]
patch v1.4

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

::: lib/utils.js
@@ +415,5 @@
>        break;
>      default:
>        var style = controller.window.getComputedStyle(element, '');
>        var state = style.getPropertyValue('visibility');
> +      visible = ((state === 'visible') || (element.hidden === 'false'));

Are the additional parentheses necessary here?

::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +39,5 @@
>      var starButton = locationBar.getElement({type: "starButton"});
>      var URI = utils.createURI(LOCAL_TEST_PAGE);
>  
> +    // Check that star button is visible
> +    utils.isDisplayed(controller, starButton);

This just returns a boolean. We need to wait for it to return true.

Comment 57

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Dave Hunt (:davehunt) from comment #54)
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #53)
> > Even if we enhance utils.isDisplayed to check for the hidden property, even
> > if the hidden property has a bad value, the test will still pass. We need to
> > re-think this properly in other bug which should probably be in another
> > sprint. 
> 
> Why would the test still pass? I don't understand why your check for
> visibility is good enough for within the test but not as part of
> isDisplayed. What other bug are you referring to here?

Can we please get a reply on it, Vlad? Or is it because you do not use waitFor() here as Dave pointed out in the last review?

Comment 58

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #57)
> (In reply to Dave Hunt (:davehunt) from comment #54)
> > (In reply to Maniac Vlad Florin (:vladmaniac) from comment #53)
> > > Even if we enhance utils.isDisplayed to check for the hidden property, even
> > > if the hidden property has a bad value, the test will still pass. We need to
> > > re-think this properly in other bug which should probably be in another
> > > sprint. 
> > 
> > Why would the test still pass? I don't understand why your check for
> > visibility is good enough for within the test but not as part of
> > isDisplayed. What other bug are you referring to here?
> 
> Can we please get a reply on it, Vlad? Or is it because you do not use
> waitFor() here as Dave pointed out in the last review?

Yup, it seems that adding the waitFor to my simple testcase fixes the issue. I will upload the revised patch now, since I'm back from my time off :)

Comment 59

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #56)
> Comment on attachment 661226 [details] [diff] [review]
> patch v1.4
> 
> Review of attachment 661226 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/utils.js
> @@ +415,5 @@
> >        break;
> >      default:
> >        var style = controller.window.getComputedStyle(element, '');
> >        var state = style.getPropertyValue('visibility');
> > +      visible = ((state === 'visible') || (element.hidden === 'false'));
> 
> Are the additional parentheses necessary here?

Not really necessary come to think of it twice now
> 
> ::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
> @@ +39,5 @@
> >      var starButton = locationBar.getElement({type: "starButton"});
> >      var URI = utils.createURI(LOCAL_TEST_PAGE);
> >  
> > +    // Check that star button is visible
> > +    utils.isDisplayed(controller, starButton);
> 
> This just returns a boolean. We need to wait for it to return true.

Yes this was a fault.

Comment 60

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.5

* fixed

Comment 61

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 662054 [details] [diff] [review]
patch v1.5

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

::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +41,5 @@
>  
> +    // Check that star button is visible
> +    controller.waitFor(function () {
> +      return utils.isDisplayed(controller, starButton);
> +    }, "Star button is displayed");

Why the unnecessary comment above? Please remove that.

Comment 62

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.6

yeah, sorry about that. removed the extra comment since its redundant with the waitfor message.
also, i did not post this for internal review since its only deletion of a comment.

Comment 63

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/d6823f76ef85

Lets check how this works for the testrun today. If it fixes our problem we can backport the patch to older branches.

Comment 64

[Maniac Vlad Florin (:vladmaniac)]

fingers crossed for this one

Comment 65

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This didn't help. So I would propose now to get this test skipped right away. No more experiments should be done here until we have the right fix. I would leave the last changeset in because it adds a valid enhancement.

Comment 66

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Or not because it would need a backport of the change which I don't think we should do right now.

Backout: http://hg.mozilla.org/qa/mozmill-tests/rev/bc127d5ba6c0

Comment 67

[Maniac Vlad Florin (:vladmaniac)]

Attachment: [default branch] skip test v1.0

* this patch was not placed for internal reviews because there is no one available at the moment that means I would have had to leave it for tomorrow

Comment 68

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 662188 [details] [diff] [review]
[default branch] skip test v1.0

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

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/b34be8955c97 (default)

Comment 69

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Transplanted skip patch as:
http://hg.mozilla.org/qa/mozmill-tests/rev/8a9c9a88f5c1 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/af7b946a5745 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/f4e48c253944 (release)

Patch does not apply cleanly to ESR10 branch. Please provide a backport patch.

Comment 70

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Backport for skip patch is not needed for mozilla-esr10 as the test does not exist on this branch. See bug 705122 for details.

Comment 71

[Maniac Vlad Florin (:vladmaniac)]

Attachment: screenshot of star UI freeze

* managed to get a screenshot of the star UI freeze before the test echoes the 'the Bookmark was created' timeout error. this is from a win xp VM, the OS where this is mostly reproducible.

Comment 72

[Maniac Vlad Florin (:vladmaniac)]

Attachment: simple test

Simple test which demonstrate that the error is manually reproducible, still intermittent. 

If we replace the test page with a page from the web, not a local one, the error is gone.

Comment 73

[Maniac Vlad Florin (:vladmaniac)]

If yo guys want to always fail the test, just bookmark a page which will redirect, for e.g

const LOCAL_TEST_FOLDER = 'http://google.ro' this will redirect to 'http://www.google.ro' and cause the test to always fail in the exact same manner.

So are we doing something wrong when using httpd with collector.addHttpResource? ..Just a guess..

Comment 74

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #71)
> * managed to get a screenshot of the star UI freeze before the test echoes
> the 'the Bookmark was created' timeout error. this is from a win xp VM, the
> OS where this is mostly reproducible.

A screenshot is not that helpful here. Way better would be a screencast to see what's actually going on.

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #73)
> If yo guys want to always fail the test, just bookmark a page which will
> redirect, for e.g
> 
> const LOCAL_TEST_FOLDER = 'http://google.ro' this will redirect to
> 'http://www.google.ro' and cause the test to always fail in the exact same
> manner.
> 
> So are we doing something wrong when using httpd with
> collector.addHttpResource? ..Just a guess..

Does httpd.js redirect to another URL? You can simply compare that. If not it's probably something else. But the above situation has to be fixed, which is a real problem and could also fix our local problem.

Comment 75

[Maniac Vlad Florin (:vladmaniac)]

httpd.js does not redirect, just tested that. Its something else and I can't figure out what

Comment 76

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Compare the URLs we are operating on. That should give you a hint.

Comment 77

[Maniac Vlad Florin (:vladmaniac)]

Attachment: simple test v2

so I am adding even a simpler test.
the problem is that the star UI does not finish loading after click
this new test will fail with "Star button changed state" timeout error.

the problem with redirected pages and how we handle those will have to be an enhancement for later, in another bug, because that is not the reason why we fail at this point.

Comment 78

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So this still does only fail for resources served via localhost?

Comment 79

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #78)
> So this still does only fail for resources served via localhost?

Yes

Comment 80

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: skip test [esr10]

Backported skip patch.

Comment 81

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 664472 [details] [diff] [review]
skip test [esr10]

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

r=me with the mentioned nits addressed.

::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +64,5 @@
>  }
> +
> +setupModule.__force_skip__ = "Bug 772360 - Mozmill test failure " +
> +                             "//testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/" +
> +                             "with error 'The bookmark was created'";

There is no need to put in the whole path of the test file. It's clear by looking at the manifest or the file name. Please update in both files. Once done it's good to go.

Comment 82

[Maniac Vlad Florin (:vladmaniac)]

Attachment: simple test v2.1

v2 was a bad file, just seen that. Uploaded v2.1 which is the good one.
The observations stand

Comment 83

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #81)
> Comment on attachment 664472 [details] [diff] [review]
> skip test [esr10]
> 
> Review of attachment 664472 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the mentioned nits addressed.
> 
> ::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
> @@ +64,5 @@
> >  }
> > +
> > +setupModule.__force_skip__ = "Bug 772360 - Mozmill test failure " +
> > +                             "//testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/" +
> > +                             "with error 'The bookmark was created'";
> 
> There is no need to put in the whole path of the test file. It's clear by
> looking at the manifest or the file name. Please update in both files. Once
> done it's good to go.

As discussed on IRC we're going to leave this as it is in the other branches.

Landed backport skip patch for mozilla-esr10:
http://hg.mozilla.org/qa/mozmill-tests/rev/8fb65f6d8e8b

Comment 84

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, can you please test the simple patch from Vlad under OS X 10.8? I don't have that platform and on 10.7 it passes all the time. I wonder if it could be an issue in Firefox on 10.8.

Comment 85

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #84)
> Dave, can you please test the simple patch from Vlad under OS X 10.8? I
> don't have that platform and on 10.7 it passes all the time. I wonder if it
> could be an issue in Firefox on 10.8.

I've reproduced it also on a win xp Vm, so 10.8 is not a must anymore

Comment 86

[Dave Hunt [:davehunt] [he/him] ⌚BST]

The sample test passes every time for me.

Comment 87

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #86)
> The sample test passes every time for me.

How many times did you run it? Eventually it will fail, it does for us here

Comment 88

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I ran it at least 10 times... can you give some indication of how many times it takes you to get it to fail? Could we update the sample test to loop so we don't have to keep running it?

Comment 89

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #88)
> I ran it at least 10 times... can you give some indication of how many times
> it takes you to get it to fail? Could we update the sample test to loop so
> we don't have to keep running it?

should have failed if you ran it 10 times. initially I've added a loop there to simulate the enduranceManager but it does not matter since it fails at start, or it does not fail at all. 
Maybe you are referring to a simple sh script to trigger the mozmill command several times and report with '>' to a local file. that's easy - want me to add the sh script in the bug?

Comment 90

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that we can't work on bug 795884 lets take this bug as replacement for the current sprint.

Comment 91

[Maniac Vlad Florin (:vladmaniac)]

The problem here is identified: we are sometimes not clicking the star element in the awesomebar. 

Details:
On windows the star UI has a hover state. Because we do not click the element, the test timeouts on the hover state. Please see here: 

chrome://browser/skin/places/bookmark.png

Once bookmarked, the style changes to 

chrome://browser/skin/places/editBookmark.png

so that it will allow you to edit the bookmark.

I've done the following so far, but without any luck in fixing this:

1. re define the element a second time in the test
2. try to use mouseUp and mouseDown instead of click()
3. try to use EventUtils.sendMouseEvent() from stdlib in mozmill instead of click
4. wait for the exact style change using window.getComputedStyle on the star element
5. controller.sleep after click on the star
6. test the simplified testcase both in a win Xp VM and on a XP local system installation, to eliminate a possible VM issue - its not VM specific failure
7. see if we have a mouse focus issue - we don't

After all that the problem is still there. I kinda ran out of ideas. I have a screencast but the file is larger that 20MB so too big for a bugzilla attachment. Dropbox will do or how do you suggest I share the file with you guys?

Thanks!

Comment 92

[Maniac Vlad Florin (:vladmaniac)]

Also default branch affected by this on Win XP at least

Comment 93

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #91)
> On windows the star UI has a hover state. Because we do not click the
> element, the test timeouts on the hover state. Please see here: 

I do not see why this will make a difference. The hover state is totally unrelated to our test. Why do you think that it is the problem? In my view the reason is still unknown.

> After all that the problem is still there. I kinda ran out of ideas. I have
> a screencast but the file is larger that 20MB so too big for a bugzilla
> attachment. Dropbox will do or how do you suggest I share the file with you
> guys?

Don't ask, just do it. I don't care how you share it but that you do it.

Comment 94

[Maniac Vlad Florin (:vladmaniac)]

I did not say that the hover state is the cause, just mentioned it because the test freezes in that state, due to not clicking on the element, which is the cause.

Comment 95

[Maniac Vlad Florin (:vladmaniac)]

Screencast here, due to comment 93:
https://www.dropbox.com/s/y5ye1imttcdenre/failure_screencast.wmv

The failure is intermittent so please be patient at the end of the video to see it.
Thanks!

Comment 96

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I've been able to replicate this on Windows XP. I can make it fail every time by adding a loop to the simple test case (I'll attach an update shortly). It appears that after a page load the star button is still 'starred' for a moment, and if we click it during this time then it will have no affect.

I was able to make the test pass 100% of the time by first waiting for the star button to be not 'starred' before clicking it. I will also attach the 'fixed' version. Hoping this can finally lead to us fixing this bug.

Comment 97

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Minimised passing and failing testcases

Comment 98

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Minimised passing and failing testcases

Comment 99

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Minimised passing and failing testcases

Corrected a typo.

Comment 100

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great investigation Dave! Checking the patch I don't like that we have to check for the starring attribute. I still feel that's too unreliable. Wouldn't opening about:blank after each iteration help here? The star will be definitely not starred.

Comment 101

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #100)
> Great investigation Dave! Checking the patch I don't like that we have to
> check for the starring attribute. I still feel that's too unreliable.
> Wouldn't opening about:blank after each iteration help here? The star will
> be definitely not starred.

This will not work, because even though the star button is not visible after loading about:blank, it is still present and still has the same 'starred' attribute value. After testing, even visiting an intermediate local page that is not bookmarked is not enough. I personally believe it's acceptable to wait for a button to not have this attribute before clicking it. Alternatively, we should not return from waitForPageToLoad before the status of this button has settled.

Comment 102

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I see. So are you saying it is a glitch in the UI? Could it be a regression in Firefox? We haven't seen it from the beginning when the test has been checked in, or am I wrong here?

Comment 103

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #100)
> Great investigation Dave! Checking the patch I don't like that we have to
> check for the starring attribute. I still feel that's too unreliable.
> Wouldn't opening about:blank after each iteration help here? The star will
> be definitely not starred.

If you don't want to check for the starred attribute you might try '4' in comment 91

Comment 104

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #102)
> I see. So are you saying it is a glitch in the UI? Could it be a regression
> in Firefox? We haven't seen it from the beginning when the test has been
> checked in, or am I wrong here?

You are right the test was checked in in February, the bug is bug 705122

Comment 105

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Vlad: This is still your bug. I'm hoping my investigation is helpful, but I haven't reassigned it to myself. I just wanted to make some progress here.

Comment 106

[Maniac Vlad Florin (:vladmaniac)]

I was testing the simple tests from Dave and both the passing and the failing testcases fail intermittently for me

I am still in digging stage with this one

Comment 107

[Maniac Vlad Florin (:vladmaniac)]

Attachment: failing stackstrace

as discussed in the weekly meeting, I am seeing intermittent failures on the simplified testcase which is supposed to pass. 

this is on my local win XP VM

attaching the stacktrace as a .txt file

Comment 108

[Daniela Petrovici]

Ran minimized test case on Windows XP with Nightly build - the test is failing once out of 100 runs.

I will continue to investigate it more.

Comment 109

[Daniela Petrovici]

Attachment: patch v2.0

Steps performed to investigate this:
1) Ran the minimized test case (that should always pass) fails intermittently both on normal and heavily loaded system
2) Increased the wait time to 10 seconds and still the test fails intermittently (3 out of 100 times)
3) Added controller.sleep(100) and it fails once out of 500 times on Windows
4) The problem seems to be the fact that the controller.click(starButton) does not work properly - cannot always click on the star button and button hasn't the correct attribute (starred)
5) Ran the test with while(!starButton.getNode().hasAttribute("starred")) controller.click(starButton). This showed that:
    - On Ubuntu: after trying to click twice, the star button is clicked and has the attribute starred
    - On Windows XP (out of 100 runs): clicking on the star button does not work in 16 of 100 cases. After a minimum of two tries at click and a maximum of 100, the star button is clicked and has the attribute starred.

Based on this investigation I have added the click function inside the waitFor.

The patch is added. I have ran it against: Ubuntu 12.04, Windows XP SP3, MAC OS 10.7.5 with the Nightly build on a normal loaded system for 100 runs and it passes every time.

Comment 110

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 683565 [details] [diff] [review]
patch v2.0

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

Good investigation, but I don't feel that this is the best approach. This could mask a genuine regression in Firefox if the star button didn't reliably add a bookmark. We need to find something appropriate to wait for. As mentioned in previous comments, I got much better reliability by initially waiting for the button not to have the starred attribute, as after a page load this appeared to linger a little too long sometimes.

::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +48,5 @@
>  
>      // Trigger editBookmarksPanel and remove bookmark
>      controller.click(starButton);
>      locationBar.editBookmarksPanel.waitForPanel();
> +    var removeBookmark = locationBar.editBookmarksPanel.getElement({type: "removeButton"});

Please restore the newline before this line.

@@ +53,2 @@
>  
> +    controller.waitFor(function () {

Why introduce a waitFor here? Our issue was with the initial click of the star button. Unless you found other issues? If so, please provide details.

@@ +57,4 @@
>  
>      controller.click(removeBookmark);
>  
> +    controller.waitFor(function () {

Same here, why introduce the waitFor?

Comment 111

[Daniela Petrovici]

Since we need to find something appropriate to wait for, I tried to run again on MAC OS 10.7.5 with a clean repo:
- with an older build (from 11/13) the issue reproduces once out of 100 times (single runs)
- with nightly build (from 11/25) the issue did not reproduce - tries 300 times (single runs)

Started running through the testrun on Mac with latest nightly build to see if it does not reproduce.

In case it does not reproduce, I will try to find a regression range or another OS where this can be reproduced

Comment 112

[Daniela Petrovici]

Based on the conversation in the meeting, I will try to find a build on which this reproduces constantly. Verify minimized test case on that build and create patch in case the fix is ok.

Comment 113

[Daniela Petrovici]

Attachment: log for the run

I have ran the test against Windows XP SP3 with rev/36427d4b2cf6 FF 18.0a1 from 09/07. The test reproduces once out of 100 times.

I have added a waitFor star button to not be starred anymore before clicking on it and also dumped all the attributes the star button has.

When everything is working properly the following is displayed:
1) Attributes of starButton before clicking on it - which is correct:
id with value: star-button
class with value: urlbar-icon
onclick with value: PlacesStarButton.onClick(event);
tooltiptext with value: Bookmark this page
2) Attributes of starButton after clicking on it:
id with value: star-button
class with value: urlbar-icon
onclick with value: PlacesStarButton.onClick(event);
tooltiptext with value: Edit this bookmark
starred with value: true

So the only two attributes that are changing at click are tooltiptext and starred

When the test is not working properly:
1) Attributes of starButton before clicking on it - which is correct:
id with value: star-button
class with value: urlbar-icon
onclick with value: PlacesStarButton.onClick(event);
tooltiptext with value: Bookmark this page

2) Attributes of starButton after clicking on it:
id with value: star-button
class with value: urlbar-icon
onclick with value: PlacesStarButton.onClick(event);
tooltiptext with value: Bookmark this page

So, when the test fails, no attribute of the starButton is changing to show that the click actually happened.

Comment 114

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Daniela Petrovici from comment #113)
> I have added a waitFor star button to not be starred anymore before clicking
> on it and also dumped all the attributes the star button has.

How does that look like? Keep in mind that dumping DOM attributes only does not mean you can catch everything. Most properties have to be accessed via the JS object like elem.getNode().localName or even via the CSS computed style.

> When the test is not working properly:
> 1) Attributes of starButton before clicking on it - which is correct:
> id with value: star-button
> class with value: urlbar-icon
> onclick with value: PlacesStarButton.onClick(event);
> tooltiptext with value: Bookmark this page
> 
> 2) Attributes of starButton after clicking on it:
> id with value: star-button
> class with value: urlbar-icon
> onclick with value: PlacesStarButton.onClick(event);
> tooltiptext with value: Bookmark this page

So that's what we already know since the bug has been filed. It's clear that the starred property hasn't been set given that we do not click the star button correctly.

Comment 115

[Daniela Petrovici]

Yesterday it was twice in three runs.
(In reply to Henrik Skupin (:whimboo) from comment #114)
> (In reply to Daniela Petrovici from comment #113)
> > I have added a waitFor star button to not be starred anymore before clicking
> > on it and also dumped all the attributes the star button has.
> 
> How does that look like? Keep in mind that dumping DOM attributes only does
> not mean you can catch everything. Most properties have to be accessed via
> the JS object like elem.getNode().localName or even via the CSS computed
> style.

When I added the comment the code looked like the below. I will try with properties view JS object and CSS computed style, too and update the bug.

    var array;
    var i;
       
    array = starButton.getNode().attributes;
   
    // Wait for the bookmark element
   
    controller.waitFor(function () {
        array = starButton.getNode().attributes;
        i = 0;
        while(i < array.length){
            dump("\FIRST[" + i + "] = " + array[i].name + " with value: " + array[i].value + "\n");
            i++;
    }
    return !starButton.getNode().hasAttribute("starred");
    }, "The bookmark isn't created");
   
    controller.click(starButton);
   
    // Wait for the bookmark event
    controller.waitFor(function () {
        array = starButton.getNode().attributes;
        i = 0;
        while(i < array.length){
            dump("\nSECOND[" + i + "] = " + array[i].name + " with value: " + array[i].value + "\n");
            i++;
        }
      return starButton.getNode().hasAttribute("starred");
    }, "The bookmark was created");

Comment 116

[Daniela Petrovici]

Attachment: test that passes with all css computed style and JS object properties

Comment 117

[Daniela Petrovici]

Attachment: test that fails with all css computed style and js object properties

I verified and there aren't any properties / attributes that change after clicking on the star button when test fails.

I listed in the attached two files all that appears in CSS Computed Style and JS Object Properties for the star-button element

Comment 118

[Daniela Petrovici]

Attachment: code used for running the tests

This is the code I have used. Please tell me if you want me to try something different.

Comment 119

[Daniela Petrovici]

Attachment: code snippet and logs

Per the discussion over IRC, I have automatically compared the arrays and added the log files for when the test passes (49.txt) and when it failed (50.txt). There isn't any difference between the JS Object properties, CSS Computed Style properties and attributes when the test fails.

Comment 120

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Daniela, have you had the time to check Marco's response from 2 days ago? I wonder if that fixes our problem.

Comment 121

[Daniela Petrovici]

I have displayed the _pendingStmt state (the code I used is below), but the issue is that it is always true before and after the click.

I am not sure I have used it properly though. Please tell me if you think I missed something.

Added the following in my test
starButton.getNode()._pendingStmt = places.asyncGetBookmarkIdsForURI(URI, function(){dump("\n\n");});
dump("\nIn WaitFor: " + !!(starButton.getNode()._pendingStmt) + ", hasAttribute: " + starButton.getNode().hasAttribute("starred") + "\n");

And added in places.js library:
Components.utils.import("resource://gre/modules/PlacesUtils.jsm");
and a function:
function asyncGetBookmarkIdsForURI(aURI, aCallback){
  dump("\n In Places \n");
  return PlacesUtils.asyncGetBookmarkIds(aURI, aCallback);
}

Comment 122

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Daniela Petrovici from comment #121)
> starButton.getNode()._pendingStmt = places.asyncGetBookmarkIdsForURI(URI,
> function(){dump("\n\n");});
> dump("\nIn WaitFor: " + !!(starButton.getNode()._pendingStmt) + ",
> hasAttribute: " + starButton.getNode().hasAttribute("starred") + "\n");

why are you modifying '_pendingStmt' of the star button? That should only be done internally by places but not by yourself or the test. You should only read its state.

Comment 123

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

As I stated in the mail, _pendingStmt is a property of the global PlacesStarButton object. This object exists in every browser window object. If you absolutely need something on the dom node, we may put an "updating" attribute on the star button, or find another solution.

Comment 124

[Daniela Petrovici]

I have dumped the value of the _pendingStmt before and after click using the code below:

dump("\nBefore click: " + !!(starButton.getNode()._pendingStmt) + "\n");
dump("\nAfter click: " + !!(starButton.getNode()._pendingStmt) + "\n");

The logs for the test when it passes and when it fails show the value of _pendingStmt is false. Please tell me if I should change something or if I am doing something wrong.

Comment 125

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

you are still dumping the wrong object, starButton.getNode() is a dom node, _pendingStmt is not a property of the node.

what you want to dump is likely (Henrik can probably confirm or fix this)
controller.window.PlacesStarButton._pendingStmt

Comment 126

[Daniela Petrovici]

I have added the dump. 

The controller.window.PlacesStarButton._pendingStmt is true before click action when the test fails. 

I have added a waitFor this element before clicking on the star-button. Code looks like:

controller.waitFor(function () {
  return !controller.window.PlacesStarButton._pendingStmt;
});

controller.click(starButton);

I am running the test and no fails so far.

Comment 127

[Daniela Petrovici]

Attachment: patch v3.0

Test passes on the older version of Firefox Nightly - 18.0a1 from 09/07 where it failed intermittently without the fix:
http://mozmill-crowd.blargon7.com/#/endurance/report/a11aa7b15f4e571fd6fe21a2db8221fe

Test passes also with today's Nightly version (20.0a1):
Ran on Ubuntu x86: http://mozmill-crowd.blargon7.com/#/endurance/report/a11aa7b15f4e571fd6fe21a2db8352b4
Ran on Windows XP SP3: http://mozmill-crowd.blargon7.com/#/endurance/report/a11aa7b15f4e571fd6fe21a2db8311cb
Ran on MAC 10.7.5: http://mozmill-crowd.blargon7.com/#/endurance/report/a11aa7b15f4e571fd6fe21a2db83f883

Comment 128

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693543 [details] [diff] [review]
patch v3.0

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

Looks fine in general and good to see it fixed. But you also want to enable the test.

::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +40,5 @@
>      var URI = utils.createURI(LOCAL_TEST_PAGE);
>  
> +    controller.waitFor(function () {
> +      return !controller.window.PlacesStarButton._pendingStmt;
> +    });

Please put this into the places module so it can be used by other tests too.

Comment 129

[Daniela Petrovici]

Attachment: patch v3.1

Added the controller.window.PlacesStarButton._pendingStmt inside lib/places to be used in other tests, too.

Comment 130

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693817 [details] [diff] [review]
patch v3.1

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

::: lib/places.js
@@ +61,5 @@
>  
>  /**
> + * Get the _pendingStmt value to verify if we can click on the star-button element
> + */
> +function getPendingStmt(aController) {

I don't think this is a good name for this method. Given that it is closely tied to the bookmarking action it should be named that way.

Comment 131

[Daniela Petrovici]

Attachment: patch v3.2

Changed the name of the function to show that it is related to the bookmark star-button and the click action

Comment 132

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 693860 [details] [diff] [review]
patch v3.2

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

Great work on this, hopefully we can land this fix soon - just a couple of minor issues remaining.

::: lib/places.js
@@ +61,5 @@
>  
>  /**
> + * Get the _pendingStmt value to verify if we can click on the star-button element
> + */
> +function isBookmarkStarButtonClickable(aController) {

Can you rename this to isBookmarkStarButtonReady

@@ +62,5 @@
>  /**
> + * Get the _pendingStmt value to verify if we can click on the star-button element
> + */
> +function isBookmarkStarButtonClickable(aController) {
> +  return aController.window.PlacesStarButton._pendingStmt;

This should return the opposite to be true when the button is ready.

::: tests/endurance/testBookmarks_AddAndRemoveBookmarkViaAwesomeBar/test1.js
@@ +39,5 @@
>      var starButton = locationBar.getElement({type: "starButton"});
>      var URI = utils.createURI(LOCAL_TEST_PAGE);
>  
> +    controller.waitFor(function () {
> +      return !places.isBookmarkStarButtonClickable(controller);

See previous comment, we shouldn't be flipping the value here. The method should return true to indicate the button is ready.

Comment 133

[Daniela Petrovici]

Attachment: patch v3.3

Changed based on review. Also observed that the functions in places were not alphabetically sorted so I changed that too.

Comment 134

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 693900 [details] [diff] [review]
patch v3.3

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

Looks good, however I missed in the previous review that the manifest change is missing. Please remove the disabling of this test from the manifest.

Comment 135

[Daniela Petrovici]

Attachment: patch v3.4

changed manifest.ini too

Comment 136

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 693932 [details] [diff] [review]
patch v3.4

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

http://hg.mozilla.org/qa/mozmill-tests/rev/5ae731622807

Comment 137

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Watch out for regressions. If it works now lets get it backported to any other branch.

Comment 138

[Daniela Petrovici]

The issue did not reproduce on Nightly builds until now.

Comment 139

[Daniela Petrovici]

Attachment: patch v1.0 for esr10 branch

Created patch for esr10 branch. The reports are below:
Linux: http://mozmill-crowd.blargon7.com/#/endurance/report/87220f551d53bd61286b3d93d916639e
Mac: http://mozmill-crowd.blargon7.com/#/endurance/report/87220f551d53bd61286b3d93d916aeae
Windows: http://mozmill-crowd.blargon7.com/#/endurance/report/87220f551d53bd61286b3d93d9166a76

Comment 140

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 696282 [details] [diff] [review]
patch v1.0 for esr10 branch

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

Great that we had no failures. Lets see on ESR10!

Comment 141

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The original patch fails to apply on all the other branches except esr10 where a different patch has been uploaded. Daniela, haven't you checked the other branches?

That's what I get:
applying 5ae731622807
patching file lib/places.js
Hunk #1 FAILED at 75
1 out of 2 hunks FAILED -- saving rejects to file lib/places.js.rej
patch failed to apply

I'm not going to land the esr10 patch until the more recent branches have been fixed.

Comment 142

[Daniela Petrovici]

Attachment: patch v1.0 for aurora branch

I have previously not checked the other branches since they had tracking flags set to disabled.

I have created a patch for aurora branch now and it applies clearly on beta, release and esr17 branches.

Comment 143

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Disabled means that the test is skipped on those branches. Esr flags are different because there is no disabled value available.

Comment 144

[Daniela Petrovici]

Attachment: patch v1.1 for aurora

Just realized that I forgot to remove the skip for the test on Aurora branch as was requested for default in comment #128.

I verified and this patch clearly applies on beta, release and esr17 branches

Comment 145

[Henrik Skupin [:whimboo][⌚️UTC+1]]

http://hg.mozilla.org/qa/mozmill-tests/rev/cda0116763bb (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/fb44b5e62909 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/d3abc4894148 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/aaabe9e3f2a8 (esr17)
http://hg.mozilla.org/qa/mozmill-tests/rev/eb9d07d6bcee (esr10)