1394101 - Port Bug 1374282 [Switch to async/await from Task.jsm/yield] to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[K Chayka]

Attachment: screenshot of error console

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 SeaMonkey/2.54a1
Build ID: 20170815055359

Steps to reproduce:

Starting with the 2017-08-16 build (both Win32 and Win64), Data Manager appears broken.
1. Start the browser
2. In the location bar, enter about:data


Actual results:

Data Manager is blank. The error console shows 3 errors (will attach screenshot):
ReferenceError:gDataman is not defined
ReferenceError:gTabs is not defined
Syntax Error: yield expression is only valid in generators

The 2017-08-26 build also shows:
FullZoom is undefined


Expected results:

Expected Data Manager to show the domains and what is stored for each.

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1394101-yield-WIP.patch

The startup error is gone but still need to do the Data Manager patch.

Comment 2

[Frank-Rainer Grahl (:frg)]

Attachment: 1394101-yield-WIP.patch

Comment 3

[Frank-Rainer Grahl (:frg)]

Attachment: 1394101-yield.patch

Ran the script from bug 1374282 against suite. This only left the Data Manager and these two were an easy fix. Looks like just the declarations to make it generator funtions were missing. I wonder if this was an error before or I miss something? 

Tested and working as far as I see it.

Comment 4

[Frank-Rainer Grahl (:frg)]

Attachment: 1394101-yield.patch

Comment adjusted. Bug number was missing. Patch not changed.

Comment 5

[K Chayka]

Is this bug the likely reason form field "suggestions" also stopped working?

Comment 6

[Frank-Rainer Grahl (:frg)]

Probably but might be Bug 1392929. They are working in my local build with both patches applied.

Comment 7

[Ian Neal]

Comment on attachment 8902003 [details] [diff] [review]
1394101-yield.patch

>+++ b/suite/common/dataman/dataman.js
>-    function loader() {
>+    function* loader() {

>-    function loader() {
>+
>+    function* loader() {

Are these changes to do with Task/yield as I cannot find similar in the reference patch.

>+++ b/suite/common/places/tests/autocomplete/test_enabled.js
>@@ -27,17 +27,17 @@ var gTests = [
>   ["3: resume normal search",
>    "url", [0], () => setSearch(1)],
> ];
> 
> function setSearch(aSearch) {
>   prefs.setBoolPref("browser.urlbar.autocomplete.enabled", !!aSearch);
> }
> 
>-add_task(function* test_sync_enabled() {
>+add_task(async function test_sync_enabled() {
>   // Initialize autocomplete component.
>   Cc["@mozilla.org/autocomplete/search;1?name=history"]
>     .getService(Ci.mozIPlacesAutoComplete);
> 
>   let types = [ "history", "bookmark", "openpage" ];
> 
>   // Test the service keeps browser.urlbar.autocomplete.enabled synchronized
>   // with browser.urlbar.suggest prefs.
Is this change correct as there is no yield in the function?

r=me with those points answered/addressed

Comment 8

[Frank-Rainer Grahl (:frg)]

> Are these changes to do with Task/yield as I cannot find similar in the reference patch.

I think the code was incorrect but there were many changes in this area and I didn't find the bug where syntax checking was tighted up. I am splitting up the patch into two parts and change the comment. If needed we can take this on to other trees or back it out and put in a new comment.

> Is this change correct as there is no yield in the function?

The corresponding file is
mozilla/toolkit/components/places/tests/unifiedcomplete/test_enabled.js

It was changed in Bug 1353542
https://bug1353542.bmoattachments.org/attachment.cgi?id=8865250

I believe the test is and was broken and needs to be adapted. Previously maybe called as a generator but I am not seeing it now.

I will add Bug 1353542 to the comment. This seem to be the main bug for the changes.

Comment 9

[Frank-Rainer Grahl (:frg)]

Attachment: 1394101-part1-yield.patch

Split patch Part 1 addressing reviewer comments. r+ from IanN carried forward.

Drat! Thought I had time till Monday but Merges just started. Well I wanted it in former Beta which is now release anyway :)

[Approval Request Comment]
Regression caused by (bug #): 1394101
User impact if declined: 2.54 broken
Testing completed (on m-c, etc.): c-c c-b
Risk to taking this patch (and alternatives if risky): low risk. Tested.
String changes made by this patch: none.

Comment 10

[Frank-Rainer Grahl (:frg)]

Attachment: 1394101-part2-yield.patch

Split patch Part 2 for Data Manager. Addressing reviewer comments. r+ from IanN carried forward.

[Approval Request Comment]
Regression caused by (bug #): 1394101
User impact if declined: 2.54 Data Manager broken
Testing completed (on m-c, etc.): c-c c-b
Risk to taking this patch (and alternatives if risky): low risk. Tested.
String changes made by this patch: none.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/56a768de610b
Part 1 Port Bug 1374282 [Switch to async/await from Task.jsm/yield] to SeaMonkey. r=IanN
https://hg.mozilla.org/comm-central/rev/264f1743605c
Part 2 Add missing generator function declare in Data Manager. r=IanN

Comment 12

[Ian Neal]

Comment on attachment 8911203 [details] [diff] [review]
1394101-part1-yield.patch

a=me

Comment 13

[Ian Neal]

Comment on attachment 8911205 [details] [diff] [review]
1394101-part2-yield.patch

a=me

Comment 14

[Frank-Rainer Grahl (:frg)]

part 1
https://hg.mozilla.org/releases/comm-beta/rev/7ccb550f060e70551fde6309b96789f157190f8f
https://hg.mozilla.org/releases/comm-release/rev/1f21d91b6b924a0326a34aee81fa67d0bd45278a

part 2 
https://hg.mozilla.org/releases/comm-beta/rev/f001cdeb5a34fda154a0c66e56e985c732ee7341
https://hg.mozilla.org/releases/comm-release/rev/b8b1280b35a3831b86f9ec14e5fb449c1e1e49b3