Closed Bug 839996 Opened 11 years ago Closed 7 years ago

Add event to wait for before importing the default bookmarks through importFromURL function

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: daniela.p98911, Unassigned)

References

Details

Attachments

(6 files, 1 obsolete file)

patch v1.0
1.69 KB, patch
Details | Diff | Splinter Review
the minimized test case that always fails (test_always_fails.js)
3.36 KB, application/javascript
Details
test_always_passing.js
7.62 KB, application/javascript
Details
test that uses local pages loaded through a web server
3.28 KB, application/javascript
Details
test always passing without any libraries imported and using remote pages
8.00 KB, application/javascript
Details
test failing on Nightly
3.36 KB, application/javascript
Details 
There is a mozmill test that keeps failing due to the fact that we are not importing the default bookmarks properly. The test is failing in tearDown module where we are just performing:
1) close all opened tabs
2) restore the default bookmarks

Code: https://hg.mozilla.org/qa/mozmill-tests/file/945cbb978d10/tests/endurance/testBookmarks_OpenAllInTabs/test1.js#l33

After the investigation we found that by adding a sleep of 0.1 seconds before performing the import in places.js library the test is not failing anymore.

Code: https://hg.mozilla.org/qa/mozmill-tests/file/945cbb978d10/lib/places.js#l102

Due to this, I think we need to wait for an event before importing the default bookmarks. The event should show if we are ready to import the bookmarks or not.

NOTES:
We have also tried increasing the timeout for import and checking for failures at import. The test still fails with these changes.
Blocks: 781547
No longer blocks: 781547
Blocks: 781547
Marco, can you please let us know if this can be implemented?
Flags: needinfo?(mak77)
I can make a test patch that you can test, sure.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)
Attached patch patch v1.0 — — Splinter Review 
This patch adds a places-browser-init-complete notification.

Apart trying if this patch helps, could you please try also to wait for places-init-complete AND additionally setTimeout(0) before proceeding?
These 2 methods may actually end up being the same and if we can avoid a further notification it's better.
Flags: needinfo?(dpetrovici)
I am not sure if I have performed the changes correctly, so please tell me if I missed something. These are the steps I have performed:
- cloned mozilla-central
- added the patch in the bug
- built Firefox
- added an observer with topic = "places-browser-init-complete" and waited for the event to occur before importing the bookmarks.

The event does not happen.

Also, I am not sure where to set the timeout to 0, do you want me to add it when waiting for the import to happen?
Flags: needinfo?(dpetrovici)
(In reply to Daniela Petrovici from comment #4)
> The event does not happen.

This may mean you are far after initialization, that means figuring out why you need a timeout there is a complete different story, basically we should restart investigation from scratch.

> Also, I am not sure where to set the timeout to 0, do you want me to add it
> when waiting for the import to happen?

no, instead of waiting for the new notification you'd wait for the old one (places-init-complete) and then just do a setTimeout(0) to be sure to run after all the places-init-complete observers.
Though at this point I doubt will make any difference.
I can restart the investigation from scratch, but what we have found is this:
- it happens in the tearDown part of the test case that has only closeAllTabs and restoreDefaultBookmarks
- if we add a sleep of 0.1 seconds between these two methods the test is working properly
- adding an observer for bookmarks-restore-failed topic does not help either, because when the issue reproduces, the bookmarks-restore-failed event does not happen => so, the restore is not failed.
- increasing the timeout when waiting for the bookmarks to be restore does not help, the test still fails intermittently

So, if we need to restart the investigation from scratch, can you please give me a hint on where should I start?
(In reply to Daniela Petrovici from comment #6)
> - if we add a sleep of 0.1 seconds between these two methods the test is
> working properly

What happens when we skip the closeAllTabs call? Does the issue persist?

> - adding an observer for bookmarks-restore-failed topic does not help
> either, because when the issue reproduces, the bookmarks-restore-failed
> event does not happen => so, the restore is not failed.

Does the success observer notification gets fired? If not what else?
what does tearDown do? based on the name looks like some shutdown thing, when does it run in the browser life?
tearDown() method cleans after the test even if the test is failing. It runs after the test is finished and it is run before the browser is closed.

Also, I did not try closeAllTabs call yet, it takes some time for the issue to reproduce locally, but I will update the bug when I have the results.

Regarding the success observer notification, it does not get fired when the error appears.
so the harness invokes tearDown when a test is complete, and when teardown is complete it tries to close the browser process (or just the window but the process survives?)
When tearDown is complete the browser process is closed, not just the window.
I suppose at this point the only way to debug this is to debug the BookmarkHTMLUtils code and see what happens there, even just with simple dump()s if needed.
Daniela, do we have a minimized testcase which let us reproduce this better?
I couldn't find a minimized test case for this issue to reproduce. It reproduces under heavy load on MAC OS once out of about 300 times by running the endurance testrun with only this test.

I have removed the closeAllTabs() from before the places.restoreDefaultBookmarks() - per comment #7 and it passes in 800 runs. I will try changing so that we restore the default bookmarks before closing all tabs, but I am not sure why it happens since tests/endurance/testBookmarks_OpenAndClose/test1.js has the exact same tearDown and sets the bookmarks the same way as testBookmarks_OpenAllInTabs.
Moving down closeAllTabs function does not help, the failure is just harder to reproduce now. So, I will try to create a minimized test case and see what is the difference between testBookmarks_OpenAndClose and testBookmarks_OpenAllInTabs since they both have the same setup and teardown and the issue does not reproduce in the first test case.
This is the investigation based on minimized test cases that are always failing or passing:
1) The current test case that always fails (test_always_fails.js) can be changed to contain a method to go through each tab and wait for all pages to load. When doing this, the test will always pass => My conclusion was that we were failing to restore default bookmarks when pages are not loaded.
2) The current test case that always fails (test_always_fails.js) can be changed to use only remote pages (not local as it did). When doing this, the test will always pass (test_always_passing.js) even if we are NOT waiting for the pages to load completely.
3) I have also created the test case test_trial.js that has only one difference between the test_always_fail.js. It uses the a local test page loaded through XAMPP local web server instead of collector.addHttpResource. The test is always passing.

So, I think that the issue is with addHttpResource:
const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
const LOCAL_TEST_PAGE = LOCAL_TEST_FOLDER + 'layout/mozilla.html?entity=';

Should we use remote pages then?
Attachment #717081 - Attachment is obsolete: true
Attached file test_always_passing.js — 

    
    

    
  
(In reply to Daniela Petrovici from comment #16)
> Should we use remote pages then?

Please stay on focus. This bug is not about our Mozmill test but a strange behavior in Firefox. Thanks.

    
    

    
  
yep, it's not important to workaround the issue, but rather to somehow make a small standalone testcase we can run to debug it, or directly debug the issue (even by adding simple dump()s to the firefox code if necessary)

    
    

    
  
The tests that I have attached are the minimized tests that I have created to reproduce the problem. Please tell me if I can provide any additional information here.

    
    

    
  
how do I run those tests locally? may you point me to simple documentation?

    
    

    
  
The test is created for running against MAC OS 10.7.5 and Beta 4 19.0. This is the latest on which our issue reproduced. To get it working locally:
1) Install mozmill: https://developer.mozilla.org/en-US/docs/Mozmill with the below commands:
$ curl -O http://peak.telecommunity.com/dist/ez_setup.py
$ sudo python ez_setup.py
$ sudo easy_install pip
$ sudo pip install mozmill

2) Clone our repo:
hg clone http://hg.mozilla.org/qa/mozmill-tests
cd mozmill-tests
hg up -C mozilla-beta

3) Copy the test to tests/endurance/<any_folder>
4) Run from terminal with mozmill -t tests/endurance/<any_folder>/test_always_fails.js -b <Beta_Build_Location> --show-errors

    
    

    
  
OK, I will try to reproduce it that way
Flags: needinfo?(mak77)

    
    

    
  


  
This is a failing test for latest Nightly and MAC OS 10.7.5. To run it:

1) Install mozmill: https://developer.mozilla.org/en-US/docs/Mozmill with the below commands:
$ curl -O http://peak.telecommunity.com/dist/ez_setup.py
$ sudo python ez_setup.py
$ sudo easy_install pip
$ sudo pip install mozmill

2) Clone our repo:
hg clone http://hg.mozilla.org/qa/mozmill-tests

3) Copy the test to tests/endurance/<any_folder>
4) Run from terminal with mozmill -t tests/endurance/<any_folder>/test_fails.js -b <Nightly_Build_Location> --show-errors

    
    

    
  
(In reply to Daniela Petrovici from comment #26)
> Created attachment 727162 [details]
> test failing on Nightly
> 
> This is a failing test for latest Nightly and MAC OS 10.7.5. To run it:
> 
> 1) Install mozmill: https://developer.mozilla.org/en-US/docs/Mozmill with
> the below commands:
> $ curl -O http://peak.telecommunity.com/dist/ez_setup.py
> $ sudo python ez_setup.py
> $ sudo easy_install pip
> $ sudo pip install mozmill
> 
> 2) Clone our repo:
> hg clone http://hg.mozilla.org/qa/mozmill-tests
> 
> 3) Copy the test to tests/endurance/<any_folder>
> 4) Run from terminal with mozmill -t
> tests/endurance/<any_folder>/test_fails.js -b <Nightly_Build_Location>
> --show-errors

Forgot to mention that for easily reproducing on Nightly, you need CPU at about 95%

    
    

    
  
For whatever reason I'm unable to run mozmill tests on my Mac by following all of the instructions (also looked at mdn instructions). The build starts, the first tab keeps loading until timeout, running from the current tip.
Maybe an unfortunate changeset, some recent change breaking tests? I may retry later unless you have ideas on what may be broken. I can't even run existing tests.

I also took a second look at the code and the test and couldn't find anything wrong.  The nature of the failure makes very hard and time expensive to reproduce it. Having to repeat the test hundreds of times with very specific cpu/disk load is a pain.

I'm not really optimist here and I feel like in the end the cost of figuring out this race condition is not balanced by a valuable gain, since this is an issue not reported by users so far, nor by any other harness. If we'd have a simpler testcase it may be worth it, but building that is also very expensive (trial and error iterations).
I'm not sure anymore it's worth depleting many other hours trying to figure the point of failure, Henrik what do you think?
Flags: needinfo?(mak77)

    
    

    
  
I chatted with Marco on IRC and the above instructions will not work given that 'pip install mozmill' installs 2.0rc2 now. The testcase attached here is for Mozmill 1.5.21. I forwarded him the link to the mozmill-env we currently are using. We were not able to finish it off last night so I will work with Marco today in getting this produced.

    
    

    
  
not actively working on this, if it's still important please add it to fxdesktoptriage and express a priority explanation
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: