Closed Bug 950441 Opened 10 years ago Closed 10 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.3 C1/1.4 S1(20dec)

People

(Reporter: gwagner, Assigned: markh, NeedInfo)

References

Details

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
Bug 935793 broke it.
Blocks: 935793
(note that all "em" are stripped out from travis output, for some reason)
Gregor, note that "fireMessageHandler" is defined in some of the mocks. Not sure what's happening...
(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.
Gregor, I meant I don't get why bug 935793 broke it here.
Maybe James knows whats going on.
What is travis test ?
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?
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
            );
        }
      }
James is in PTO right now, I'm gonna needinfo the other marionette js client peers for moving this forward.
Flags: needinfo?(mike)
Flags: needinfo?(gaye)
(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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
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.
Flags: needinfo?(mike)
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.
Flags: needinfo?(mike)
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).
Flags: needinfo?(mike)
You need to log in before you can comment on or make changes to this bug.