473976 - MozMill test breaks editBookmark popup initialization

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

Attachment: MozMill test which will break editBookmark popup (zip)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090116 Shiretoko/3.1b3pre Ubiquity/0.1.4 ID:20090116020328 and MozMill latest trunk

Running the attached Mozmill test under "firefox/bookmarks" results in breaking the editBookmarks popup. I don't know why that happens and if this is a bug in MozMill or a core issue. Mikeal or Adam, could you both please have a look at the test?

If this is a core issue we should move it to the appropriate product/component later.

Comment 1

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

Attachment: Screenshot of failure

Comment 2

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

One further note. This issue can only be seen once within a Firefox session. You have to restart Firefox to see it again. Means my test it not totally wrong, but probably shows an issue which normally will not appear when manual testing this behavior.

Comment 3

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

Running the test for the first time shows following error and exceptions:

Error: Error: could not find element ID: editBMPanel_folderMenuList
Source File: file:///Volumes/Daten/mozilla/profiles/mozmill/extensions/[e-mail]/resource/modules/frame.js
Line: 342

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getFolderIdForItem]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/editBookmarkOverlay.js :: EIO_initPanel :: line 157"  data: no]

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getItemTitle]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/editBookmarkOverlay.js :: EIO__getItemStaticTitle :: line 415"  data: no]

Comment 4

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

Ok, I've updated my code and waiting for editBMPanel_folderMenuList element before clicking on it. But I still think that this shouldn't break the initialization of the dialog.

  // Use menu to open the bookmark panel
  controller.menus.Bookmarks["Bookmark This Page"].click();
  controller.waitForElement(new elementslib.ID(controller.window.document, "editBMPanel_folderMenuList"));

  // Bookmark URI under bookmarks menu
  controller.click(new elementslib.ID(controller.window.document, "editBMPanel_folderMenuList"));

Comment 5

[Adam Christian]

This looks like a situation where when you try to access this node editBMPanel_folderMenuList before it's really done rendering, you can actually break it (or whatever code it is that is building it).

I simply did this:
  controller.menus.Bookmarks["Bookmark This Page"].click();
  controller.sleep(1000)

The one second seems to get you over that little gap where things are still drawing. I don't necessarily think this is a bug in Windmill, but I will look at editBookmarkOverlay.js line 415 and see if there is anything we can automatically do to avoid this.

Comment 6

[Adam Christian]

Can someone tell me where I can find editBookmarkOverlay.js in a web accessible source repo?

Comment 7

[Dietrich Ayala (:dietrich)]

you can search the source repo by filename (or a substring of) like so:

http://mxr.mozilla.org/mozilla-central/find?string=editbookmarkoverlay.js

Comment 8

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

Sure, you should use MXR to search on mozilla-central. Just enter the file name and hit search: http://mxr.mozilla.org/mozilla-central/

You will end up here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/editBookmarkOverlay.js

Probably Marco can help us here, when he is back in an hour or so.

Adam, I think using the waitforElement is a better way here and that also works for me without having to wait useless ~700ms.

Comment 9

[Adam Christian]

That would be great, except on my machine your test using waitForElement is still broken the same way as if it's not there and is reproducibly broken. I tried many things and literally the only thing that I can do to get mine to not be broken is a 500-1000ms static sleep. 

Thanks for the pointers on searching the moz source.

Comment 10

[Marco Bonardo [:mak]]

so a small explanation of what happens when you click on the star or generally when you open a bookmark edit / add panel.

The editBookmarkPanel is already present in all dialogs from the begin, it's part of the xul that defines the dialog, could hwv be that its contents are lazy loaded when first accessed, that could explain the mozmill first error.

About the places errors, practically the internal editBookmarkPanel is initialized after the star panel, when you click the star (or open any other bookmarks dialog/panel) there is a small initialization of the external panel/dialog, then a bookmark is added through places api, and FINALLY the editBookmarkPanel gets initialized. (See http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#169)
So if you try to use the panel before it is initialized (through a call to initPanel), it cannot know what bookmark you're working for, internally its _itemId property is -1, and all calls to places backend will fail (and throw). Also the elements in the panel are collapsed at init, so you see them all since it was not inited. (see http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/editBookmarkOverlay.js#126)
This should explain the 2 remaining places errors that you see.

So what you should wait is that the panel is initialized, not that true... we could maybe add an attribute to editBookmarkPanel that will show its status, and you could poll it until you see it has been initialized.

Comment 11

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

Adam, do we have a function, which I can use to poll a given attribute?

Comment 12

[Adam Christian]

We have the waitForEval function that takes a javascript expression and waits until it evaluates to true, we could do something as follows:

waitForEval("subject.property == true", 1000, 100, editBookmarkPanelObject);

So I certainly think the property would be a good solution to this, thanks Marco for the explanation and possible solutions!

Comment 13

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

Mmh using that function in the following way does not work:

controller.waitForElement(new elementslib.ID(controller.window.document, "editBookmarkPanel"));
  controller.waitForEval("subject.property == true", 1000, 100, editBookmarkPanel);

While the first call succeeds the second call to waitForEval states:
"exception: ReferenceError: editBookmarkPanel is not defined"

Comment 14

[Adam Christian]

that was just an example, subject would need to be a property that gets set when its finished lazy loading, it would be something like editBookmarkPanel.loaded

Comment 15

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

(In reply to comment #10)
> So what you should wait is that the panel is initialized, not that true... we
> could maybe add an attribute to editBookmarkPanel that will show its status,
> and you could poll it until you see it has been initialized.

That would be great. Something like "editBookmarkPanel.loaded" what Adam already proposed?

Comment 16

[Marco Bonardo [:mak]]

Attachment: patch v1.0

this should allow for:

controller.waitForEval("subject.initialized == 'true'",1000, 100, new elementslib.ID(controller.window.document, "editBookmarkPanelContent"));

Comment 17

[Dietrich Ayala (:dietrich)]

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

r=me

Comment 18

[Dão Gottwald [:dao]]

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

>+  _panel: null,
>+  get panel() {
>+    if (!this._panel)
>+      this._panel = document.getElementById("editBookmarkPanelContent");
>+    return this._panel;
>   },

Since this is isn't a prototype object, you can use this pattern:

>  get panel() {
>    delete this.panel;
>    return this.panel = document.getElementById("editBookmarkPanelContent");
>  },

Comment 19

[Marco Bonardo [:mak]]

Attachment: patch v1.1

addressed dao's comment

Comment 20

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/d11dfa373acf

Comment 21

[Marco Bonardo [:mak]]

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

asking approval, low risk

Comment 22

[Marco Bonardo [:mak]]

i backed this out because it was not working as expected (still some strange behavior due to mozmill interaction with a lazy binding probably) and this added back the Library leak. Drew has created a test for the leak in bug 474831.

Comment 23

[Marco Bonardo [:mak]]

backout: http://hg.mozilla.org/mozilla-central/rev/89911edfc032
merge: http://hg.mozilla.org/mozilla-central/rev/8dd6a5e00031

Comment 24

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

Marco, we called the waitForEval function with the wrong object. The element which gets returned by elementslib is an elementslib object but not the real one, which shall be inspected. We have to call .getNode() to get the real element like you can see here: 

let elem = new elementslib.ID(controller.window.document, "editBookmarkPanel");
controller.waitForEval("subject.initialized === true", 1000, 100, editBookmarkPanel.getNode());

Using that way we should be able to correctly check for the initialized property.

Comment 25

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

Sorry, hit Enter too early. Here the correct code:

let e = new elementslib.ID(controller.window.document, "editBookmarkPanel");
controller.waitForEval("subject.initialized === true", 1000, 100, e.getNode());

Comment 26

[Marco Bonardo [:mak]]

thanks for clarification, still before going on we need 2 leak tests, one from bug 474831 and one from bug 433231.

Comment 27

[Marco Bonardo [:mak]]

i was just now looking at the code for the star UI, the internal overlay is loaded dynamically here http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#166
so it's not available on first open, but the starUI object in the top window has a property _overlayLoaded that you could look at

Comment 28

[Marco Bonardo [:mak]]

so both the overlay is loaded lazily, and the internal panel (object gEditItemOverlay) is initialized later

Comment 29

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

Attachment: Using StarUI for waitForEval

Thanks for this great information Marco! Even I would feel better to use an element which is returned by the MozMill inspector, the given property from the StarUI works fantastic.

That means we don't need a fix for the editBookmarksPanel right now. But if you think an update would simplify the query for other modules, feel free to give us an improvement.

Clint, what do you think? It's a simple fix which prevent us from running into a timeout each time.

Comment 30

[Marco Bonardo [:mak]]

at that point, you could also look at gEditItemOverlay.itemId and check its value is > 0 (that global object should exist in the window when the overlay is loaded)

Comment 31

[cmtalbert]

Comment on attachment 362457 [details] [diff] [review]
Using StarUI for waitForEval

r= ctalbert  This looks really good.  Thanks whimboo!

Comment 32

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

Pushed to mozmill-tests:
http://hg.mozilla.org/qa/mozmill-tests/rev/374c15ba1f06

Marco, any enhancements should to the panel itself should be handled in a new bug.

Comment 33

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv