Closed Bug 1223070 Opened 7 years ago Closed 7 years ago

Intermittent apps/homescreen/test/marionette/bookmark_order_test.js | Homescreen - Bookmark order Bookmark order is retained after restart


(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Gonk (Firefox OS)
Not set


(Not tracked)

2.6 S1 - 11/20


(Reporter: gwagner, Assigned: mikehenrty)



(Keywords: intermittent-failure, Whiteboard: [systemsfe])

No description provided.
Whiteboard: [systemsfe]
Blocks: 1222215 is an attempt to fix this. Without logs, I'm just trying stuff blindly, though. I'm assuming that this won't reproduce when the tests are run locally, so I created the PR just as a way to get the tests running on treeherder
That attempted fix did not work. 

I notice that bookmark_test.js is also failing most of the time with "Crash detected but error running stackwalk", and bookmark_test.js is similar to, but simpler than bookmark_order_test.js, so it probably makes sense to try to fix that one and hope that the fix also fixes this.
I added console.log() calls to bookmark_test.js, and now see this in the logs on treeherder:

server created
TEST-START | apps/homescreen/test/marionette/bookmark_test.js | Homescreen - Bookmark Bookmarking adds bookmark to homescreen
got home
got system
created Bookmark
system fully loaded
homescreen launched
homescreen has: 49
bookmarking http://localhost:43428/sample.html
switching to system app
TEST-UNEXPECTED-FAIL | apps/homescreen/test/marionette/bookmark_test.js | Homescreen - Bookmark Bookmarking adds bookmark to homescreen
Crash detected but error running stackwalk
TEST-END | apps/homescreen/test/marionette/bookmark_test.js | Homescreen - Bookmark Bookmarking adds bookmark to homescreen took 23300 ms

I see "switching to system app" which is the output right before the call to bookmark.openAndSave(url), but I do not see the output from the console.log() right after that, so I'm guessing the failure is in system/test/marionette/lib/bookmark.js
With logging in bookmark.js it looks like the test is failing at bookmark.js:125;
In bookmark_test.js, if I put a try/catch around bookmark.openAndSave(), then the test does not crash, but fails with a timeout because no bookmark was ever opened or saved.

The exception that is thrown is:

{ [NoSuchElement: NoSuchElement: Unable to locate element: [data-id=add-to-homescreen], Remote Stack: <none>]

So it is failing to find an element it needs, and for some reason this exception does not have a stack associated with it. I suppose that is why I was seeing the "Crash detected but error running stackwalk" message without the try/catch.

The exception is coming from bookmark.js where openAndSave() is defined, that function then calls the appChromeContextMenuBookmark getter function in system.js, and that is where the exception is really coming from, I think.

But that getter function calls waitForElement(), so I don't get why it is failing instead of waiting. I suppose it could be timing out.

Bug 1179579 recently changed the CSS selector that is used for appChromeContextMenuBookmark, so I tried reverting that change thinking that this bug might be a regression from that change. But that didn't make a difference. With both the old and new selectors, we're getting exceptions here.
So this code at system.js:185:

    return this.client.helper.waitForElement(

Is timing out after 20 seconds and is failing with that NoSuchElement exception that does not include a stack trace.

Maybe that will be enough for someone who knows the code well to figure out what is going on. I'm not sure I'll be able to make any more progress with it.

(Incidentally, I don't get why this is an intermittent failure. It looks to me like it is failing every single time. I found this bug because it was on the list of dependencies for 1222215, so I thought it would pass most of the time and was only exposed now that we stopped automatically repeating the tests.)
Ah ha! The problem here seems to be related to this code in system/js/browser_context_menu.js:

    _getSaveUrlItem: function(url, name) {
      if (this.pinningEnabled) {
        return {
          id: 'pin-to-home-screen',
          label: _('pin-to-home-screen'),
          callback: this.pinUrl.bind(this, url, name)

      return {
        id: 'add-to-homescreen',
        label: _('add-link-to-home-screen'),
        callback: this.bookmarkUrl.bind(this, url, name)

We used to use 'add-to-homescreen' as the context menu element id, but now we use 'pin-to-home-screen'. If I make that change to the selector in system.js, the test gets further along before failing.  Now it gets as far as calling bookmark.add() and now the test times out in the currentTabFrame getter method waiting for the element with selector ' iframe[mozbrowser]',

I don't know enough about how bookmarking and pinning work, but it looks to me like the test failures in bookmark_test.js and bookmark_order_test.js are caused by the switch to pin-the-web.  Either the tests need to be updated to test pinning, or the tests need to somehow force the system to run with its old bookmarking behavior instead of its new pinning behavior.

Michael: I think I've diagnosed the problem here, but I don't know how to fix it. Setting needinfo for you because you started this, and because I think you'll know how to fix it or will know who can.
Flags: needinfo?(mhenretty)
Wow David, thank you for the careful investigation.

The thing is that in the test, we disable "pinning the web", so the "add-to-homescreen" context menu item should be appearing instead of "pin-to-home-screen." As you mention, the fact that the selector ` [data-id=add-to-homescreen]` is not found seems to be the real issue, but removing the `:not(.homescreen)` part is not what we want since we want to wait until the homescreen appWindow becomes inactive, and the browser window (with proper context menu) becomes active. So the failure happens because this browser window never becomes active during some runs. I'm not sure why :/

But in any case, I think you are right that we should just convert this test to a pinning test since the new homescreen is not set up to support bookmarks anyway. I'll take this one.

Assignee: nobody → mhenretty
Flags: needinfo?(mhenretty)
Target Milestone: --- → 2.6 S1 - 11/20
Depends on: 1223778
Fixed this by converting the test to use pinning rather than bookmarks.
Closed: 7 years ago
Resolution: --- → FIXED

Thanks! Most of my testing above was on bookmark_test.js because it was failing in exactly the same way as bookmark_order_test.js. So I think that one needs to be fixed in the same way.  I don't know if it already has a bug filed.
Flags: needinfo?(mhenretty)
No we don't have a bug file for that just yet, but that was the test I was going to look at next. I think the problem is that the bookmark marionette library has had to work with both the old and new homescreen, and because of that it is flakey in the new homescreen. It's probably best to just convert all the bookmark tests, which is indeed the goal of bug 1215524.
Flags: needinfo?(mhenretty)
You need to log in before you can comment on or make changes to this bug.