776189 - bug 771826 breaks Marionette's navigate() method

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Jonathan Griffin (:jgriffin)]

Attachment: test case

bug 771826 has broken Marionette's navigate method, which is implemented here:

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#494

Specifically, setting curWindow.location no longer has any effect (and produces no errors in the logfile); before https://hg.mozilla.org/mozilla-central/rev/934ef44ce5af it worked normally.  See bug 776117 for full bisection details.

To reproduce:

 - download a debug build from TBPL, or make a build yourself with ENABLE_MARIONETTE=1 in mozconfig
 - launch Firefox, and add the pref marionette.defaultPrefs.enabled = true; restart Firefox
 - go to m-c/testing/marionette/client and run 'python setup.py develop' to install Marionette Python dependencies
 - download the attached test and execute:  python navigate.py

results:  Marionette opens a new tab (as it should) but the navigate command has no effect.  There are no errors in the logfile.
expected results:  After opening a new tab, the browser would navigate immediately to www.mozilla.com.

In stepping through Marionette code, everything works correctly until http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#495; setting curWindow.location now has no effect.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

What is "curWindow" when that code runs? Does it point to the main browser content window, or some other window? The only possibly relevant change that I can see in bug 755136 is that it added a hidden <browser> to browser.xul, which may be somehow interfering with the marionette loading code.

I don't really understand how marionette works, though, so I think you'll have to debug this further. How does startBrowser get called in this case? Does it get called multiple times? Does the content script load in the social sidebar <browser>?

Comment 2

[Jonathan Griffin (:jgriffin)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #1)
> What is "curWindow" when that code runs? Does it point to the main browser
> content window, or some other window? The only possibly relevant change that
> I can see in bug 755136 is that it added a hidden <browser> to browser.xul,
> which may be somehow interfering with the marionette loading code.
> 
> I don't really understand how marionette works, though, so I think you'll
> have to debug this further. How does startBrowser get called in this case?
> Does it get called multiple times? Does the content script load in the
> social sidebar <browser>?

curWindow is the standard 'content' variable available to content frame scripts (https://developer.mozilla.org/en/The_message_manager#Content_scripts).

I'll try to debug this a little more; perhaps Marionette is attaching to the new hidden <browser> element instead of the "real" one.

Comment 3

[Jonathan Griffin (:jgriffin)]

The culprit is definitely this commit:  https://hg.mozilla.org/mozilla-central/raw-rev/934ef44ce5af

If I start with the previous commit and then apply that patch, the problem manifests.  If I then remove this panel element:

https://hg.mozilla.org/mozilla-central/rev/934ef44ce5af#l3.12

...the problem disappears.  So it definitely has something to do with the way this browser element is used.

When Marionette starts a new session, it opens a new tab, see http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-actors.js#1491.  If I change this.startPage from 'about:blank' to 'http://www.google.com', I see a curious result:  the browser opens the new tab, and the title of this tab says 'http://www.google.com', but the page is never loaded.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

I can confirm that in that test case 'curWindow' is the window associated with the 'social-notification-browser' browser element.  I haven't yet worked out why that window is being used though - I'll look again in the (my) morning.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

The problem seems to be caused by a combination of factors:

* The social work has added 2 new <browser> elements with type="content" (and they must be "content" as they must not have chrome permissions).  It appears the MessageManager stuff only works on browsers with type="content-*" - hence the problem doesn't exist for, eg, the <browser> element for the normal sidebar - that is created without a type= attribute, so defaults to type="chrome".

* marionette creates a new tab, but seems to rely on specific ordering of the "Marionette:register" messages - the very first such message it sees with a href of "about:blank" is set as the "curFrameId" and becomes the target for all future marionette messages for the session.

The addition of the new <browser> elements means that the first 2 elements which have marionette-listener injected into them are these new elements.  The "correct" window in this example is now the third content window that gets injected and hence sends marionette:register.  As the first is used, one of the new <browser> elements becomes the target of the marionette messages (ie, in the test case, it is one of these new browser elements that has its href set to mozilla.org)

There is alot I don't understand about marionette, particularly why the new tab isn't automagically associated with the session.  However, it does seem that all the windows of interest to marionette have a type of "content-primary" or "content-targetable" rather than plain "content" - so a work-around is to change the handling of Marionette:register message to do something like:

        let browserType;
        try {
            let container = frameObject.document.defaultView.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebNavigation).QueryInterface(Ci.nsIDocShell).chromeEventHandler;
            browserType = container.getAttribute("type");
        } catch (ex) {
            // browserType remains undefined.
        }
        let reg;
        if (!browserType || browserType != "content") {
            reg = this.curBrowser.register(message.json.value, message.json.href);

ie, so attempt to locate the containing <browser> element for the window and only call the .register method if type != "content".  With this change the testcase works, but I doubt that really is the most appropriate way to fix this.  I think we will need to drag in some people who understand marionette more than I do.

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Mark Hammond (:markh) from comment #5)
> ie, so attempt to locate the containing <browser> element for the window and
> only call the .register method if type != "content".  With this change the
> testcase works, but I doubt that really is the most appropriate way to fix
> this.  I think we will need to drag in some people who understand marionette
> more than I do.

Thanks for the clarification! From what you described, it seems like the right answer is to have the browser of type!="content" to be selected as the curWindow whenever we add a new tab, navigate, etc. But we also want the user to have access to the type="content" browsers if they want to, so I think the solution here is, whenever we load a page, we add all type="content" browsers to some list, and set the browser of type!="content" as the curWindow. Then, we can let the user call a method like "switchToContentWindow" which will let them switch contexts to the type="content" browsers if they so wish.

David Burns mentioned that we would have to double check this approach in the B2G environment, since we are unsure of what kind of browsers are used there.

Jgriffin, thoughts?

Comment 7

[David Burns :automatedtester]

I have verified that this works especially for instances where we need to move from window to window e.g. Persona (BrowserID). I have a patch ready to go based on comment 5.

Comment 8

[David Burns :automatedtester]

Attachment: Fixed based on Mark's comment

Comment 9

[Jonathan Griffin (:jgriffin)]

This fix works fine in B2G.

Comment 10

[Jonathan Griffin (:jgriffin)]

Attachment: patch v0.2

Slightly cleaner patch.  I think if we want to support operating in secondary content windows, we should handle that in a separate bug.

Comment 12

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 646419 [details] [diff] [review]
patch v0.2

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

lgtm, I guess we can address the different content windows when needed.

Comment 13

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/mozilla-central/rev/08428deb1e89