576940 - Mozmill needs updating for the component manager registration changes.

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Mark Banner (:standard8)]

I think the MozMill suite needs updating for the component manager registration changes. As far as I can tell, I think this is just jsbridge that needs changing, but I could be wrong.

Symptoms:

- We've been patching up Thunderbird in bug 575740, to the extent that it now runs reasonably well (some patches are not committed yet).

- Run mozmill in the way we've always done ("make mozmill")

- Thunderbird starts up and so does mozrunner at the command line

- After a short period, 10 - 20 seconds? We get the following error:

Traceback (most recent call last):
  File "runtest.py", line 391, in <module>
    ThunderTestCLI().run()
  File "/Library/Python/2.5/site-packages/mozmill-1.4-py2.5.egg/mozmill/__init__.py", line 518, in run
    self._run()
  File "/Library/Python/2.5/site-packages/mozmill-1.4-py2.5.egg/mozmill/__init__.py", line 468, in _run
    self.mozmill.start(runner=runner, profile=runner.profile)
  File "/Library/Python/2.5/site-packages/mozmill-1.4-py2.5.egg/mozmill/__init__.py", line 162, in start
    self.create_network()
  File "/Library/Python/2.5/site-packages/mozmill-1.4-py2.5.egg/mozmill/__init__.py", line 142, in create_network
    self.jsbridge_port)
  File "/Library/Python/2.5/site-packages/jsbridge-2.3.4-py2.5.egg/jsbridge/__init__.py", line 72, in wait_and_create_network
    raise Exception("Sorry, cannot connect to jsbridge extension")
Exception: Sorry, cannot connect to jsbridge extension

From what I can tell, there's no related errors on the javascript console, or in the debug even though the extensions (jsbridge, mozmill) seem to be registered correctly. This could also be a bug in core I guess, but raising here in the first instance.

Comment 1

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

Clint, we should fix jsbridge ASAP and send out an interim version before we release Mozmill 1.4.2. Can we get this done tomorrow?

Comment 2

[Mark Banner (:standard8)]

Attachment: WIP fix

This seems to fix mozmill for me on trunk. I'm pretty sure it breaks branches, but that should be a matter of combining the comment outs with the things I've added.

I may have missed components as well, but this seems to work for Thunderbird trunk.

Comment 3

[Philip Chee]

This is what I did for the Console2 command line handler:

try
{
  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
}
catch(e) { }

/**
* XPCOMUtils.generateNSGetFactory was introduced in Mozilla 2 (Firefox 4).
* XPCOMUtils.generateNSGetModule is for Mozilla 1.9.1 (Firefox 3.5).
*/

if ("undefined" == typeof XPCOMUtils) // Firefox <= 2.0
{
  function NSGetModule(aComMgr, aFileSpec)
  {
    return new Console2Handler();
  }
}
else if (XPCOMUtils.generateNSGetFactory)
  var NSGetFactory = XPCOMUtils.generateNSGetFactory([Console2Handler]);
else
  var NSGetModule = XPCOMUtils.generateNSGetModule([Console2Handler]);

Comment 4

[Mark Banner (:standard8)]

Attachment: The fix

This seems to work for Thunderbird tests on Gecko 1.9.1, 1.9.2 and 2.0.

Strictly speaking I think the httpd.js changes aren't necessary - but I've kept them roughly in sync with the mozilla-central version of the file and hence haven't included fallbacks there.

I also updated the minversions as we're basically dropping gecko 1.9.0 and older here (we could support them, but I see no reason to).

Comment 5

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

(In reply to comment #4)
> Strictly speaking I think the httpd.js changes aren't necessary - but I've kept
> them roughly in sync with the mozilla-central version of the file and hence
> haven't included fallbacks there.

Can you please remove this part from your patch? Under mozilla/scripts we have a sync script which will fetch the newest version and applies our own patch on it. We should handle it separately and I can come up with a fix for it.

> I also updated the minversions as we're basically dropping gecko 1.9.0 and
> older here (we could support them, but I see no reason to).

I thought gecko 1.9.0 doesn't work with Mozmill. Interesting. We never officially supported Firefox 3.0.x. So I think that we are fine here.

Comment 6

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

Attachment: httpd only + patch update [checked-in]

That patch uses all the updates in httpd.js from m-c and updates our local patch.

Comment 7

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

Comment on attachment 456880 [details] [diff] [review]
The fix

I have applied the patch to my local tree and it works fine with Firefox builds of all branches.

>           <em:id>[e-mail]</em:id>
>-          <em:minVersion>1.9</em:minVersion>
>-          <em:maxVersion>1.9.*</em:maxVersion>
>+          <em:minVersion>1.9.1</em:minVersion>
>+          <em:maxVersion>2.0b*</em:maxVersion>

Can you please use 2.0 for the tookit maxVersion of mozmill and jsbridge?

Comment 8

[cmtalbert]

Comment on attachment 456880 [details] [diff] [review]
The fix

This looks good.  I'll check and see what AMO will let us get away with and update the max versions appropriate for checking in there.  On the tree, we should use the highest max version possible (2.0*) or what have you.

Thanks for the patch, sorry it took so long to review.

We can tweak the version changes before checking in I don't think we need a second patch.

Comment 9

[cmtalbert]

(In reply to comment #5)
> (In reply to comment #4)

> > I also updated the minversions as we're basically dropping gecko 1.9.0 and
> > older here (we could support them, but I see no reason to).
> 
> I thought gecko 1.9.0 doesn't work with Mozmill. Interesting. We never
> officially supported Firefox 3.0.x. So I think that we are fine here.
Yes, we did support 1.9.0.  However, as support for it is phasing out across the board, I'm happy to drop support for it in order to keep our code sane.

Comment 10

[cmtalbert]

Comment on attachment 456902 [details] [diff] [review]
httpd only + patch update [checked-in]

I'm happy with these changes, but I thought we had a script that did this automatically so that we didn't need to fork httpd.js and put it in the mozmill repo?  That was my understanding of your comments.  Is that not the case?

r=ctalbert if we need to take these now.  But we should file a follow on to NEVER have a duplicate copy of httpd checked in to the mozmill repo.

Comment 11

[Mark Banner (:standard8)]

Clint, I assume you can check this in for me?

Also, what's your ETA on getting a new version out with the fix in?

Comment 12

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

(In reply to comment #10)
> I'm happy with these changes, but I thought we had a script that did this
> automatically so that we didn't need to fork httpd.js and put it in the mozmill
> repo?  That was my understanding of your comments.  Is that not the case?

No. And I have no idea how to do that at the moment without digging much deeper into git docs. We would need something like a post pull hook which we would have to execute after you pull'ed the repository.

Comment 13

[Mark Banner (:standard8)]

Attachment: The fix v2 [checked-in]

With comments addressed.

Comment 14

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

Comment on attachment 457129 [details] [diff] [review]
The fix v2 [checked-in]

Landed on master and 1.4.2:
http://github.com/mozautomation/mozmill/commit/90f4aa892a96fbd5c7a535805515ad37859ec428
http://github.com/mozautomation/mozmill/commit/d6f2b77f75ae19e0db250ca5bb42224739393993

Comment 15

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

Comment on attachment 456902 [details] [diff] [review]
httpd only + patch update [checked-in]

Landed on master and 1.4.2:
http://github.com/mozautomation/mozmill/commit/997cfbcc573ce837a97f3f2058d3c86824b9ba71
http://github.com/mozautomation/mozmill/commit/42c22cb8decb8dc454311ca3bc233e834ee7527f

Comment 16

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

I get errors for jsbridge with older branches of Firefox, which I haven't seen with the first version of Marks patch:

Traceback (most recent call last):
  File "/Volumes/data/testing/envs/dev/bin/mozmill", line 8, in <module>
    load_entry_point('mozmill==1.4.1', 'console_scripts', 'mozmill')()
  File "/Volumes/data/build/tools/mozmill/mozmill/mozmill/__init__.py", line 626, in cli
    CLI().run()
  File "/Volumes/data/build/tools/mozmill/mozmill/mozmill/__init__.py", line 603, in run
    self._run()
  File "/Volumes/data/build/tools/mozmill/mozmill/mozmill/__init__.py", line 552, in _run
    self.mozmill.start(runner=runner, profile=runner.profile)
  File "/Volumes/data/build/tools/mozmill/mozmill/mozmill/__init__.py", line 211, in start
    self.appinfo = self.get_appinfo(self.bridge)
  File "/Volumes/data/build/tools/mozmill/mozmill/mozmill/__init__.py", line 264, in get_appinfo
    mozmill = jsbridge.JSObject(bridge, mozmillModuleJs)
  File "/Volumes/data/build/tools/mozmill/jsbridge/jsbridge/jsobjects.py", line 76, in __init__
    name = bridge.set(name)['data']
  File "/Volumes/data/build/tools/mozmill/jsbridge/jsbridge/network.py", line 228, in set
    return self.run(_uuid, 'bridge.set('+', '.join([encoder.encode(_uuid), obj_name])+')')
  File "/Volumes/data/build/tools/mozmill/jsbridge/jsbridge/network.py", line 202, in run
    raise JSBridgeDisconnectError("Connected disconnected")
jsbridge.network.JSBridgeDisconnectError: Connected disconnected

Comment 17

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

Thanks Clint for pushing the follow-up. As it turns out httpd.js on mozilla-central only supports the new way of the component registration:

http://hg.mozilla.org/mozilla-central/file/default/netwerk/test/httpserver/httpd.js#l5118

That means we have to possible options:
* Update our patch so we always include both ways in our forked version
* Get httpd.js updated on trunk so it will use both ways

Jeff, would the 2nd bullet point never be an option for you for the trunk version?

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> Thanks Clint for pushing the follow-up. As it turns out httpd.js on
> mozilla-central only supports the new way of the component registration:
...
> That means we have to possible options:
> * Update our patch so we always include both ways in our forked version
> * Get httpd.js updated on trunk so it will use both ways

See comment 4 - I don't think MozMill needs the httpd.js component registration changes because it creates the httpd server via makeserver (or some function like that), rather than trying to get it through xpcom.

Comment 19

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

So why has the following patch fixed the problem?

http://github.com/mozautomation/mozmill/commit/657d34a7c5a3e88a2b68ef8641d3021f263c6a90

Comment 20

[Mark Banner (:standard8)]

(In reply to comment #19)
> So why has the following patch fixed the problem?
> 
> http://github.com/mozautomation/mozmill/commit/657d34a7c5a3e88a2b68ef8641d3021f263c6a90

Well, if generateNSGetFactory didn't exist in XPCOMUtils, then it would throw on hitting that line, which means that not all of httpd.js (if any) would get loaded in.

If I'm right, then in theory you could have probably "fixed" it differently by just commenting out that line and not doing the registration at all.

In any case, we're not supplying a manifest entry for httpd.js so it just isn't going to get registered...

Comment 21

[Mark Banner (:standard8)]

Just to confirm, the latest git repo works fine for all three of our Thunderbird branches (with a couple of minor mods to our harness which are just resolving things that have changed in mozmill).

Comment 22

[cmtalbert]

(In reply to comment #18)
> (In reply to comment #17)
> > Thanks Clint for pushing the follow-up. As it turns out httpd.js on
> > mozilla-central only supports the new way of the component registration:
> ...
> > That means we have to possible options:
> > * Update our patch so we always include both ways in our forked version
> > * Get httpd.js updated on trunk so it will use both ways
> 
> See comment 4 - I don't think MozMill needs the httpd.js component registration
> changes because it creates the httpd server via makeserver (or some function
> like that), rather than trying to get it through xpcom.
You're right about that.  I didn't realize we didn't have the httpd.js stuff in our manifest files, I just figured we'd forgotten to do the graceful degredation for generateNSGetFactory.  Oh well, no harm to leave that in our fork for now. I'm working on generating a new set of packages on pypi, should have that done in a moment.  But I think we can close this bug as FIXED now that the code has landed.

Comment 23

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

(In reply to comment #22)
> You're right about that.  I didn't realize we didn't have the httpd.js stuff in
> our manifest files, I just figured we'd forgotten to do the graceful
> degredation for generateNSGetFactory.  Oh well, no harm to leave that in our
> fork for now. I'm working on generating a new set of packages on pypi, should

So how should the patch for httpd look like? Shall I use your changes therefor, Clint?

Comment 24

[cmtalbert]

What do you mean?  For the real httpd.js?

Comment 25

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

(In reply to comment #24)
> What do you mean?  For the real httpd.js?

No for https.patch:
http://github.com/whimboo/mozmill/blob/master/mozmill/patches/httpd.patch

Comment 26

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

Marking as verified fixed, now that it works perfectly for a couple of weeks.

(In reply to comment #25)
> (In reply to comment #24)
> > What do you mean?  For the real httpd.js?
> 
> No for https.patch:
> http://github.com/whimboo/mozmill/blob/master/mozmill/patches/httpd.patch

Lets follow-up with this topic on bug 583888.