950441 - Constant Travis test failure with JavaScriptError: (17) TypeError: window.wrappedJSObject.fireMessageHandler is not a function

Status: RESOLVED FIXED

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

Description

[Gregor Wagner [:gwagner]]

This seems to be a gecko regression. The regression window is 
https://hg.mozilla.org/mozilla-central/pushloghtml?startID=25830&endID=25831

The travis failure looks like:
https://travis-ci.org/mozilla-b2g/gaia/jobs/15460525

It is also reproduces locally with: 'APP=email make test-integration' in the gaia folder.

Nothing really sticks out here. Very wild guesses are 944847 or 944407?
The function in question is here: https://github.com/mozilla-b2g/gaia/search?q=fireMessageHandler&ref=cmdform

Comment 1

[Gregor Wagner [:gwagner]]

Bug 935793 broke it.

Comment 2

[Julien Wajsberg [:julienw]]

(note that all "em" are stripped out from travis output, for some reason)

Comment 3

[Julien Wajsberg [:julienw]]

Gregor, note that "fireMessageHandler" is defined in some of the mocks. Not sure what's happening...

Comment 4

[Gregor Wagner [:gwagner]]

(In reply to Julien Wajsberg [:julienw] from comment #2)
> (note that all "em" are stripped out from travis output, for some reason)

The error msg is fine locally.(In reply to Julien Wajsberg [:julienw] from comment #3)
> Gregor, note that "fireMessageHandler" is defined in some of the mocks. Not
> sure what's happening...

Bisect shows that bug 935793 causes the failure.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Bug 935793 backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e2de961acc

Comment 6

[Julien Wajsberg [:julienw]]

Gregor, I meant I don't get why bug 935793 broke it here.

Comment 7

[Gregor Wagner [:gwagner]]

Maybe James knows whats going on.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

What is travis test ?

Comment 9

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

I suspect the problem is:

https://github.com/mozilla-b2g/marionette-content-script/blob/0bd1b2334d4d312707fb6330040433ec66096f9c/index.js#L21 - this code is looking for the "old" observer names before bug 935793 renamed them.

James, any suggestions for how we can land bug 935793 and a fix for the marionette code in a coordinated fashion?

Comment 10

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

hrm - that code includes:

Services.obs.addObserver(
          this, 'remote-browser-frame-show', false
        );

note the missing 'n' from the observer name.  Best I can tell, this code isn't going to be hit - so only the 'in-process-browser-or-app-frame-shown' case will be hit.

Ignoring this and fixing the apparent typo in the observer name, one option would be to change the relevant functions in this file to what I've pasted (untested) below.  This should allow the code to work in both the "old" and "new" cases (and obviously the code handling the "old" cases should then be removed after the other bug lands.)  Obviously, if it transpires that 'in-process-browser-or-app-frame-shown' really is the only case we care about, this would need tweaking.

      init: function() {
        // These notifications will be sent up until bug 935793 lands.
        Services.obs.addObserver(
          this, 'remote-browser-frame-shown', false
        );

        Services.obs.addObserver(
          this, 'in-process-browser-or-app-frame-shown', false
        );
        // These notifications will be sent after bug 935793 lands.
        Services.obs.addObserver(
          this, 'remote-browser-shown', false
        );

        Services.obs.addObserver(
          this, 'inprocess-browser-shown', false
        );
      },

      observe: function(subject, topic, data) {
        var frameLoader = subject.QueryInterface(Ci.nsIFrameLoader);
        switch (topic) {
          case 'remote-browser-shown':
          case 'inprocess-browser-shown':
            // These notifications will come when bug 935793 lands, but 
            // they will be sent for all frames - we only want ones coming
            // from a "browser or app" frame
            if (!frameLoader.ownerIsBrowserOrAppFrame) {
              return;
            }
          // fall through...
          default:
            var mm = frameLoader.messageManager;

            mm.loadFrameScript(
              dataURI,
              true
            );
        }
      }

Comment 11

[Julien Wajsberg [:julienw]]

James is in PTO right now, I'm gonna needinfo the other marionette js client peers for moving this forward.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #5)
> Bug 935793 backed out.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a4e2de961acc

https://hg.mozilla.org/mozilla-central/rev/a4e2de961acc

Comment 13

[Mike Pennisi [:jugglinmike]]

Hi Julien: it looks like :RyanVM resolved this by backing out the platform regression caused by that patch for bug 935793. This makes me think that there's no Marionette client work to be done; let me know if I'm mistaken.

Comment 14

[Julien Wajsberg [:julienw]]

But Bug 935793 needs to eventually land, and will need a change in the marionette client to land ;)

Please see comment 10 for more information.

Comment 15

[Mike Pennisi [:jugglinmike]]

Ah, now I understand! Sorry about that, Julien--I've opened bug 952176 to track that work (and marked it as blocking bug 935793, as you might expect).