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

Status: RESOLVED FIXED

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

Comment 1

[David Flanagan [:djf]]

https://github.com/mozilla-b2g/gaia/pull/33115 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

Comment 2

[David Flanagan [:djf]]

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.

Comment 3

[David Flanagan [:djf]]

I added console.log() calls to bookmark_test.js, and now see this in the logs on treeherder:

suiteSetup
server created
TEST-START | apps/homescreen/test/marionette/bookmark_test.js | Homescreen - Bookmark Bookmarking adds bookmark to homescreen
setup
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
suiteTeardown

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

Comment 4

[David Flanagan [:djf]]

With logging in bookmark.js it looks like the test is failing at bookmark.js:125

    this.system.appChromeContextMenuBookmark.click();

Comment 5

[David Flanagan [:djf]]

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: .appWindow.active:not(.homescreen) [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.

Comment 6

[David Flanagan [:djf]]

So this code at system.js:185:

    return this.client.helper.waitForElement(
      System.Selector.appChromeContextMenuBookmark);

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.)

Comment 7

[David Flanagan [:djf]]

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 '.inline-activity.active 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.

Comment 8

[Michael Henretty [:mikehenrty][: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 `.appWindow.active:not(.homescreen) [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.


1.) https://github.com/mozilla-b2g/gaia/pull/33115/files#diff-14bc910882a7d124d4b4b3f0bac8b307R9

Comment 9

[Michael Henretty [:mikehenrty][:mhenretty]]

Fixed this by converting the test to use pinning rather than bookmarks.

Comment 10

[David Flanagan [:djf]]

Michael,

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.

Comment 11

[Michael Henretty [:mikehenrty][: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.

Comment 12

[Intermittent Failures Robot]

7 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* gaia: 7

Platform breakdown:
* gaia: 7

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1223070&startday=2015-11-09&endday=2015-11-15&tree=all