1292569 - Port |Bug 1199296 - Don't allow legacy generator yield in method definitions| to Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

First seen on Daily build 2016-08-05:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=43547
https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=43543

Massive test failures:
TEST-UNEXPECTED-FAIL | mail/base/test/unit/test_emptyTrash_dbViewWrapper.js | xpcshell return code: 0
SyntaxError: non-generator method definitions may not contain yield at ../../../../mailnews/resources/messageModifier.js:133
TEST-UNEXPECTED-FAIL | mail/base/test/unit/test_viewWrapper_imapFolder.js | xpcshell return code: 0
SyntaxError: non-generator method definitions may not contain yield at ../../../../mailnews/resources/messageModifier.js:133
TEST-UNEXPECTED-FAIL | mail/base/test/unit/test_viewWrapper_virtualFolderCustomTerm.js | xpcshell return code: 0
SyntaxError: non-generator method definitions may not contain yield at ../../../../mailnews/resources/messageModifier.js:133
TEST-UNEXPECTED-FAIL | mail/base/test/unit/test_viewWrapper_logic.js | xpcshell return code: 0
SyntaxError: non-generator method definitions may not contain yield at ../../../../mailnews/resources/messageModifier.js:133
TEST-UNEXPECTED-FAIL | mail/base/test/unit/test_viewWrapper_virtualFolder.js | xpcshell return code: 0
SyntaxError: non-generator method definitions may not contain yield at ../../../../mailnews/resources/messageModifier.js:133 

And many more.

Comment 1

[alta88]

Bug 1199296

Comment 2

[:aceman]

So do we need to do conversions like the one in the patch?
-  get nodes() {
+  *nodes() {

Comment 3

[:aceman]

And then the callers:
-    for (let node of SocialMarks.nodes) {
+    for (let node of SocialMarks.nodes()) {

Comment 4

[aleth [:aleth]]

Yes, we have to get rid of the obsolete nonstandard syntax . I wonder if there's a good way to find all instances (obviously the test failures help).

Comment 5

[:aceman]

Probably just grep for yield and look at the containing function.

I try to look at it.

Comment 6

[:aceman]

I converted some occurrences and some tests improved:
https://hg.mozilla.org/try-comm-central/rev/8aca6940ad476ff3d79b788301e4631569dbacc5

But I'm still stuck at this problem:
 EXCEPTION: folderBatch.messages is undefined
    at: messageInjection.js line 676
       add_sets_to_folders messageInjection.js:676 11
       make_new_sets_in_folders messageInjection.js:560 3
       test_folder_names_in_recent_view_mode test-folder-names-in-recent-mode.js:52 3
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestDirectory frame.js:525 7
       runTestDirectory frame.js:707 3
       Bridge.prototype._execFunction server.js:179 10
       Bridge.prototype.execFunction server.js:183 16
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3

Comment 7

[aleth [:aleth]]

https://dxr.mozilla.org/comm-central/source/mailnews/test/resources/messageInjection.js#572 should be a function*, does that help?

Comment 8

[:aceman]

The _looperator? It is function* in my patch
https://hg.mozilla.org/try-comm-central/rev/23ca1339c301ad0b87706e6b429a2e1307d7f84e

Comment 9

[:aceman]

This one could be better:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0c4b34d61665bb2f8bf9a55269762488be18d3d7

Looks like we need to use iterator.next().value, not just iterator.next(). I'll look at some more cases later in the day.

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks for looking into it, this seems like a real pain. I prefer bustage that you can fix with sed ;-)
Haven't we got any JS experts who could help/advise here?

Comment 11

[Jorg K (CEST = GMT+2)]

Mozmill on latest try is looking promising, Xpcshell also better than before. Looks like we're on the home stretch ;-)

Comment 12

[:aceman]

Some of the changes are needed in the core code. Even if we fix the tests, some surprises may still lurk while running TB normally (tests do not cover all code paths). 'Thanks' to the JS non-typed behaviour, if I change a function to proper generator, I have to hunt for all the callers manually (no compiler finds the now non-type-matching callers automatically). So there may be remnants, that may be found only by running nightlies in production.
I CC our QA guys to watch for these problems and alert us in follow-up bugs. Thanks.

Comment 13

[:aceman]

Attachment: fix for mozmill

This fixes the mozmill tests:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cb176244084fc11fb237c15a14cf9eacddf92e1c

xpcshell tests are a different beast as the tests themselves are often made as generators. And that seems to create more problems in asyncTestUtils.js if they are converted to function* . I need to find out what the conversion pattern there is.

Comment 14

[:aceman]

The patch does not cover Seamonkey as I can't test it easily.

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 8778679 [details] [diff] [review]
fix for mozmill

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

::: mail/base/modules/MailUtils.js
@@ +381,3 @@
>          timer.cancel();
>          if (aCallback)
> +          aCallback(!thrown);

Not sure this is correct. It seems to change the logic slightly. Also I'd just do the timer cancelling and calling of callback directly like before, not keeping variables (which seem to change the logic)

::: mail/test/mozmill/shared-modules/test-folder-display-helpers.js
@@ +890,5 @@
>      aController = mc;
>    // SyntheticMessageSet special-case
>    if (typeof(aViewIndex) != "number") {
> +    let msgHdrIter = aViewIndex.msgHdrs();
> +    let msgHdr = msgHdrIter.next().value;

could just be

 let msgHdr = aViewIndex.msgHdrs().next().value;

::: mailnews/db/gloda/modules/datastore.js
@@ +3168,1 @@
>                                                                aValues) {

nit: align aValues one more space

::: mailnews/db/gloda/modules/explattr.js
@@ +138,1 @@
>                                             aCallbackHandle) {

align once space

::: mailnews/db/gloda/modules/index_msg.js
@@ +3230,1 @@
>                                                       aCallbackHandle) {

align one more space

::: mailnews/test/resources/messageModifier.js
@@ +123,5 @@
>    /**
>     * @return a JS iterator of the message headers for all messages inserted into
>     *     a folder.
>     */
> +  *msgHdrs() {

Is this correct? Would have expected 

msgHdrs: function*() {

@@ +167,5 @@
>    /**
>     * @return a generator that yields [nsIMsgFolder, nsIMutableArray of the
>     *     messages from the set in that folder].
>     */
> +  *foldersWithXpcomHdrArrays() {

same thing here??

foldersWithXpcomHdrArrays: function*() {

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8778679 [details] [diff] [review]
fix for mozmill

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

::: mail/test/mozmill/downloads/test-about-downloads.js
@@ +51,5 @@
>      this.removedItems.push(aDownload.target.path);
>      this.items.delete(aDownload);
>    },
>  
> +  *waitForFinish() {

Magnus complained about something similar below. So should this be waitForFinish: function* () {

Comment 17

[aleth [:aleth]]

(In reply to Magnus Melin from comment #15)
> > +  *msgHdrs() {
> 
> Is this correct? Would have expected 
> 
> msgHdrs: function*() {

It means the same thing, just written as an ES6 method. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Method_definitions

Comment 18

[:aceman]

(In reply to Magnus Melin from comment #15)
> ::: mail/base/modules/MailUtils.js
> @@ +381,3 @@
> >          timer.cancel();
> >          if (aCallback)
> > +          aCallback(!thrown);
> 
> Not sure this is correct. It seems to change the logic slightly. Also I'd
> just do the timer cancelling and calling of callback directly like before,
> not keeping variables (which seem to change the logic)

It didn't work without this change. First it seems generators do not throw the exception StopIteration that we were expecting here. They return .done = true and .value = undefined. And StopIteration is deprecated and will be removed too.

But the change should not change the logic, just use the proper properties on generator to get the same log.
So in which case do you see a logic change?

> ::: mailnews/test/resources/messageModifier.js
> @@ +123,5 @@
> >    /**
> >     * @return a JS iterator of the message headers for all messages inserted into
> >     *     a folder.
> >     */
> > +  *msgHdrs() {
> 
> Is this correct? Would have expected 
> 
> msgHdrs: function*() {

Somehow I got the idea that it is the same.

Comment 19

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #18)
> But the change should not change the logic, just use the proper properties
> on generator to get the same log.
> So in which case do you see a logic change?

Thx for the explanation, there's no change then. I'd still not mix in the variables but just

try {
  if (worker.next().done) {
    timer.cancel();
    if (aCallback)
      aCallback(true);
  }
} catch (ex) {
  timer.cancel();
  if (aCallback)
    aCallback(false);
}

> > > +  *msgHdrs() {
> > 
> > Is this correct? Would have expected 
> > 
> > msgHdrs: function*() {
> 
> Somehow I got the idea that it is the same.

Seems they are (per aleth's link). The non-shorthand seems more readable to me though.

Comment 20

[:aceman]

(In reply to Magnus Melin from comment #19)
> Thx for the explanation, there's no change then. I'd still not mix in the
> variables but just
> 
> try {
>   if (worker.next().done) {
>     timer.cancel();
>     if (aCallback)
>       aCallback(true);
>   }
> } catch (ex) {
>   timer.cancel();
>   if (aCallback)
>     aCallback(false);
> }

OK, I wanted to avoid this code duplication but yes, I can do this so it is obvious it should be the same.
 
> > > > +  *msgHdrs() {
> > > 
> > > Is this correct? Would have expected 
> > > 
> > > msgHdrs: function*() {
> > 
> > Somehow I got the idea that it is the same.
> 
> Seems they are (per aleth's link). The non-shorthand seems more readable to
> me though.

To me too :)

Comment 21

[:aceman]

Attachment: fix for mozmill v1.1 [checked in in comment 24]

Comment 22

[Jorg K (CEST = GMT+2)]

So is this an r+ then with the nits fixed that we can land?
Any test bustage is pretty fatal since new problems are creeping in unnoticed, very recently for example:
Bug 1246447 comment #15.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8778702 [details] [diff] [review]
fix for mozmill v1.1 [checked in in comment 24]

I checked that all the complaints are addressed, so we should land this.
Not sure that Magnus is still online.

Comment 24

[Jorg K (CEST = GMT+2)]

Mozmill part landed:
https://hg.mozilla.org/comm-central/rev/7b9fbef870b2

Comment 25

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #13)
> xpcshell tests are a different beast as the tests themselves are often made
> as generators. And that seems to create more problems in asyncTestUtils.js
> if they are converted to function* . I need to find out what the conversion
> pattern there is.
I just picked one test as random: 
mailnews/imap/test/unit/test_saveImapDraft.js

In there:
function createDraftsFolder() {
  IMAPPump.incomingServer.rootFolder.createSubfolder("Drafts", null);
  yield false;

So adding the * there doesn't help?

Looks like the failed tests
https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=43628
are all the ones which include asyncTestUtils.js:
https://dxr.mozilla.org/comm-central/search?q=asyncTestUtils.js&redirect=false

I've noticed that function _async_test_runner in asyncTestUtils.js also has a few yield statements. Hmm.
[Just a few ignorant observations ;-( ]

Comment 26

[:aceman]

It does not help by itself.


The comment at async_run() in asyncTestUtils.js say that tests can be functions the return OR yield a true/false. But converting a yielding function to function* changes the syntax slightly so I need to find what to change in asyncTestUtils.js .

The main problem seems to be here:
"Unhappiness at: ../../../../mailnews/resources/asyncTestUtils.js:156"
 0:01.41 PROCESS_OUTPUT: Thread-1 (pid:11255) "Because: TypeError: curGenerator.send is not a function"

Maybe we can't call send on a real generator function.

Comment 27

[aleth [:aleth]]

(In reply to :aceman from comment #26)
> The comment at async_run() in asyncTestUtils.js say that tests can be
> functions the return OR yield a true/false. But converting a yielding
> function to function* changes the syntax slightly so I need to find what to
> change in asyncTestUtils.js .

The more modern/simpler way of doing async tests is https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests#Task-based_asynchronous_tests but it's probably quicker to just update asyncTestUtils.js.

> The main problem seems to be here:
> "Unhappiness at: ../../../../mailnews/resources/asyncTestUtils.js:156"
>  0:01.41 PROCESS_OUTPUT: Thread-1 (pid:11255) "Because: TypeError:
> curGenerator.send is not a function"
> 
> Maybe we can't call send on a real generator function.

Yes, .send is some ancient nonstandard syntax, I have no idea what it did... maybe there is still some old documentation around somewhere.

Comment 28

[:aceman]

I think rkent wanted to get rid of asyncTestUtils in the long run, so this could be the chance, if it isn't too tedious.

Yes, I also can't find documentation what function.send() does. There is only .bind(), .call(), .apply() methods described for function.prototype.

Comment 29

[:aceman]

I think I'm onto something here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6d2a7a19056d1bb74d7bebb84f758233e20b7b45

Locally all the tests passed for me in /imap /compose /news /base /mime.

Comment 30

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #12)
> So there may be remnants, that may be found only by running
> nightlies in production.
Due to Windows bustage (bug 1282256) it was impossible to run Daily TB 51a1 in production until today. I've just installed it and will look out for any problems.

Here comes the first one: Gloda. I'm using Gloda only one single folder, the folder where I move my BMO mail. After starting TB 51 and looking in the activity manager, this hangs with: Indexing messages, Determining which messages to index.

Comment 31

[Richard Marti (:Paenglab)]

Could this be this error I got?

Mon Aug 08 2016 14:04:08
Error: TypeError: this.activeIterator.close is not a function
Source file: resource://app/modules/gloda/indexer.js
Line: 897

Comment 32

[Jorg K (CEST = GMT+2)]

I think so. I was going to rebuild tonight, but Richard was quicker ;-)

Comment 33

[Philip Chee]

(In reply to :aceman from comment #14)
> The patch does not cover Seamonkey as I can't test it easily.
Is there a dummies guide? I know almost nothing about generator yields.

Comment 34

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #31)
> Mon Aug 08 2016 14:04:08
> Error: TypeError: this.activeIterator.close is not a function
> Source file: resource://app/modules/gloda/indexer.js
> Line: 897

I see it in the xpcshell tests, I'll try to fix it.

(In reply to Philip Chee from comment #33)
> (In reply to :aceman from comment #14)
> > The patch does not cover Seamonkey as I can't test it easily.
> Is there a dummies guide? I know almost nothing about generator yields.

Maybe start here:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*
Also see the type of changes my patch does (and also the other one for xpcshell on try server).

Comment 35

[:aceman]

Attachment: fix for xpcshell

This works for me locally and makes ALL xpcshell tests pass (a feat for my setup:))

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e8fbc93711ebee9b3502db9e3d5d2c13c4ac2524

Apart from just adding * everywhere (where yield is called), the real interesting change is in asyncTesUtils.js (end of patch).

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 8778702 [details] [diff] [review]
fix for mozmill v1.1 [checked in in comment 24]

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

::: mail/test/mozmill/shared-modules/test-folder-display-helpers.js
@@ -891,5 @@
>    // SyntheticMessageSet special-case
>    if (typeof(aViewIndex) != "number") {
> -    let msgHdrIter = aViewIndex.msgHdrs;
> -    let msgHdr = msgHdrIter.next();
> -    msgHdrIter.close();

As discussed on IRC, according to https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Generator this needs to be .return()

Comment 37

[:aceman]

Attachment: fix for xpcshell v1.1 [checked in in comment 39]

Slight fix to previous version, .close() -> .return().

Try run with this:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c007c31ebcd2c52db528655c24488837e168e2dc

Comment 38

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8779085 [details] [diff] [review]
fix for xpcshell v1.1 [checked in in comment 39]

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

The asterisk store called; they're running out of asterisks! ;)

Comment 39

[Jorg K (CEST = GMT+2)]

Xpcshell part landed:
https://hg.mozilla.org/comm-central/rev/378b9a66d495
I'm leaving this open for further adjustments for now.
We need to wait for the asterisk store to produce more.

Maybe Kent or Magnus want to do a post-landing review, especially here:
https://hg.mozilla.org/comm-central/rev/378b9a66d495#l105.13 and here:
https://hg.mozilla.org/comm-central/rev/378b9a66d495#l20.21 and here:
https://hg.mozilla.org/comm-central/rev/378b9a66d495#l7.12 (.close()/.return() restored from first patch)
https://hg.mozilla.org/comm-central/rev/7b9fbef870b2#l12.12 (mistakenly removed here).

Note: Patch converted mailnews/imap/test/unit/test_subfolderLocation.js from CRLF line endings to LF endings without any further changes. (Which also made the patch fail to apply on Windows. I managed.)

Comment 40

[Magnus Melin [:mkmelin]]

Comment on attachment 8779085 [details] [diff] [review]
fix for xpcshell v1.1 [checked in in comment 39]

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

::: mailnews/test/resources/asyncTestUtils.js
@@ +154,5 @@
>      curGenerator = asyncGeneratorStack[asyncGeneratorStack.length-1][0];
>      try {
> +      let nextItem = true;
> +      while (nextItem) {
> +        nextItem = curGenerator.next(asyncGeneratorSendValue);

I'd avoid mixing variable types like this. nextItem will never be anything falsy anyway, so you can just have this for clearer code

while(true) {
  let nextItem = .....

@@ +155,5 @@
>      try {
> +      let nextItem = true;
> +      while (nextItem) {
> +        nextItem = curGenerator.next(asyncGeneratorSendValue);
> +        if (!nextItem.value || nextItem.done)

We shouldn't continue just because the return value is falsy?

Comment 41

[Jorg K (CEST = GMT+2)]

Comment on attachment 8779085 [details] [diff] [review]
fix for xpcshell v1.1 [checked in in comment 39]

Sorry for the quick landing and "post-landing" review. We can land a tiny follow up patch.

Comment 42

[:aceman]

(In reply to Magnus Melin from comment #40)
> ::: mailnews/test/resources/asyncTestUtils.js
> @@ +154,5 @@
> >      curGenerator = asyncGeneratorStack[asyncGeneratorStack.length-1][0];
> >      try {
> > +      let nextItem = true;
> > +      while (nextItem) {
> > +        nextItem = curGenerator.next(asyncGeneratorSendValue);
> 
> I'd avoid mixing variable types like this. nextItem will never be anything
> falsy anyway, so you can just have this for clearer code

OK.

> while(true) {
>   let nextItem = .....

I need nextItem outside of the loop, but that is trivial.

 
> @@ +155,5 @@
> >      try {
> > +      let nextItem = true;
> > +      while (nextItem) {
> > +        nextItem = curGenerator.next(asyncGeneratorSendValue);
> > +        if (!nextItem.value || nextItem.done)
> 
> We shouldn't continue just because the return value is falsy?

I got the impression that the tests always yield true or false. On false, we should stop. Also compare to the original code. The loop stopped when .send() returned falsy, which is now replaced by .next().value. The .next().done is a replacement for the StopIteration exception. So I'd say I didn't want to change behaviour.
But I see a slight difference. Previously if it has thrown, it didn't set asyncGeneratorSendValue=undefined. I can fix that.

Comment 43

[Andrew Sutherland [:asuth] (he/him)]

(In reply to :aceman from comment #42)
> I got the impression that the tests always yield true or false. On false, we
> should stop. Also compare to the original code. The loop stopped when
> .send() returned falsy, which is now replaced by .next().value. The
> .next().done is a replacement for the StopIteration exception. So I'd say I
> didn't want to change behaviour.

Yes, the contract is defined here:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/MailNews/AsyncTestUtils_Extended_Framework#The_Asynchronous_Function_Contract

It made some sense in a pre-Promise world, but as :jcranmer has pointed out, it would have been better to come up with the Task.jsm flow instead!

Comment 44

[:aceman]

Attachment: tweak for xpcshell [checked in in comment 53]

Comment 45

[aleth [:aleth]]

Attachment: Remove legacy generators from im/ [checked in in comment 48]

Comment 46

[Jorg K (CEST = GMT+2)]

Comment on attachment 8779450 [details] [diff] [review]
tweak for xpcshell [checked in in comment 53]

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

::: mailnews/test/resources/asyncTestUtils.js
@@ +163,5 @@
> +
> +      if (nextItem.done) {
> +        asyncGeneratorStack.pop();
> +      } else {
> +        asyncGeneratorSendValue = undefined;

Doesn't that always need to be reset? Why move it in here?

Comment 47

[Jorg K (CEST = GMT+2)]

Sorry, my changes overwrote Magnus' r+. Can you add it again? If I do it, it will look as if I gave the r+.

Comment 48

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/bc541ec44b099e32dfac2eb7a196883b161d53bc
Bug 1292569 - Remove legacy generators from im/. r=clokep

Comment 49

[Magnus Melin [:mkmelin]]

Comment on attachment 8779450 [details] [diff] [review]
tweak for xpcshell [checked in in comment 53]

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

Adding back the r+. I'll leave the question for aceman ;)

Comment 50

[:aceman]

(In reply to Jorg K (GMT+2, PTO during summer) from comment #46)
> ::: mailnews/test/resources/asyncTestUtils.js
> @@ +163,5 @@
> > +
> > +      if (nextItem.done) {
> > +        asyncGeneratorStack.pop();
> > +      } else {
> > +        asyncGeneratorSendValue = undefined;
> 
> Doesn't that always need to be reset? Why move it in here?

As said, in the original code, if the .send() threw the StopIteration exception and jumped to the catch() block, it didn't set this variable. So I try to preserve that exactly. But as tests pass either way, it may not be important.

Comment 51

[aleth [:aleth]]

(In reply to :aceman from comment #50)
> As said, in the original code, if the .send() threw the StopIteration
> exception and jumped to the catch() block, it didn't set this variable. So I
> try to preserve that exactly. But as tests pass either way, it may not be
> important.

Preserving it exactly makes sense for this bug. You can get rid of asyncTestUtils altogether and replace it with Tasks in another bug ;)

Comment 52

[Jorg K (CEST = GMT+2)]

I haven't fully understood that that asyncGeneratorSendValue variable does since it's not looked at any more after being set. Perhaps it persists. But I'd sleep better the way you had it and the way is was landed and tested. If I had to decide, I'd change the loop and nothing else ;-)
Are you planning on another try or did you run some tests locally?

Comment 53

[:aceman]

https://hg.mozilla.org/comm-central/rev/cf69eee353d3126887db17c3266d4bfa8a961d63

I would now close this bug unless there are any known open issues.