Closed Bug 1272107 Opened 3 years ago Closed 3 years ago

[FlyWeb] Review Desktop changes for landing in mozilla-central

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: djvj, Assigned: justindarc)

References

Details

Attachments

(4 files, 4 obsolete files)

Attached patch flyweb-ui.patch (obsolete) — Splinter Review
Sub-bug for reviewing Desktop changes for landing in m-c.
Firefox code, let's keep this under the Firefox component.
Component: Networking → General
Product: Core → Firefox
Attached patch bug1272107.patch (obsolete) — Splinter Review
Mike, this is the patch from Bug 1265441 re-based to be applied to m-c instead of larch. I didn't want to automatically carry the R+ over without checking with you first. The only difference between this patch and the one from Bug 1265441 is that this patch doesn't contain any reverts of the old temp UI from larch.
Assignee: nobody → jdarcangelo
Attachment #8751443 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8758410 - Flags: review?(mconley)
Comment on attachment 8758410 [details] [diff] [review]
bug1272107.patch

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

It'll be good to see a try build, and do a measurement against a mozilla-central baseline via https://treeherder.mozilla.org/perf.html#/comparechooser to ensure that we don't cause any talos regressions.

::: browser/extensions/flyweb/skin/shared/flyweb.css
@@ +1,1 @@
> +#flyweb-panel {

Needs MPL license block

::: browser/extensions/flyweb/skin/windows/flyweb.css
@@ +1,1 @@
> +@import url("chrome://flyweb-shared/skin/flyweb.css");

Out of curiosity, is all of this redirection necessary, or can we just ask jar.mn to point all platforms at the shared flyweb.css?
Attachment #8758410 - Flags: review?(mconley) → review+
Attached patch bug1272107.patch (obsolete) — Splinter Review
Addressed review comments. Carrying R+ forward.
Attachment #8758410 - Attachment is obsolete: true
Attachment #8759315 - Flags: review+
Pushed by kvijayan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b65f7e689b5
FlyWeb desktop UI code.  Only enabled on nightly and aurora builds. r=mconley
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1f056b981b1f

You know how on Larch the OS X 10.10 debug e10s tests frequently hang on startup, like https://treeherder.mozilla.org/logviewer.html#?job_id=22703&repo=larch did? Not a coincidence, that's FlyWeb's fault, to the tune of three or four or six or nine per push while it was in mozilla-inbound.
Attached patch bug1272107.patch (obsolete) — Splinter Review
Mike, this patch got backed out due to intermittent failures. Here's an updated diff for the add-on code that adds a pref observer so we do nothing unless "dom.flyweb.enabled" is set. This should prevent the issue on m-c.
Attachment #8759315 - Attachment is obsolete: true
Attachment #8760378 - Flags: review?(mconley)
Attachment #8760378 - Flags: review?(mconley) → review+
Pushed by kvijayan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8dab5e5d63a
FlyWeb desktop UI.  Only enabled on nightly and aurora. r=mconley
Backed out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/322d6c120f48 but this time with either a better log, or at least one I actually looked at: https://treeherder.mozilla.org/logviewer.html#?job_id=29619749&repo=mozilla-inbound#L13017 says that rather than just a hang, it's aborts and assertion failures.
Seeing this in the full raw log:

{"thread": "ProcessReader", "level": "info", "pid": 2024, "source": "mochitest", "time": 1465259400949, "action": "log", "exc_info": false, "message": "1465259400943\taddons.xpi\tWARN\tException running bootstrap method shutdown on flyweb@mozilla.org: TypeError: gDiscoveryManagerInstance is undefined (resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/features/flyweb@mozilla.org.xpi!/bootstrap.js:202:5) JS Stack trace: uninit@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/features/flyweb@bootstrap.js:202:5 < shutdown@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///builds/slave/test/build/application/NightlyDebug.app/Contents/Resources/browser/features/flyweb@bootstrap.js:41:3 < this.XPIProvider.callBootstrapMethod@XPIProvider.jsm:4754:9 < this.XPIProvider.startup/<.observe@XPIProvider.jsm:2778:15 < SpecialPowersObserver.prototype.receiveMessage@SpecialPowersObserver.jsm:249:7 < permitUnload@remote-browser.xml:356:13 < CanCloseWindow@browser.js:6126:36 < canClose@browser.js:4982:12 < SpecialPowersObserver.prototype.receiveMessage@SpecialPowersObserver.jsm:249:7"}

Not sure if this is causing the hangs on OS X 10.10 e10s though. Either way, updated patch on its way.
Attached patch bug1272107.patchSplinter Review
This patch adds a check at shutdown to avoid cleaning up the discovery manager if it never got created in the first place. This should avoid the JS exception I mentioned in Comment 10. Still don't know if this is what is causing the startup hangs on OS X 10.10 e10s.
Attachment #8760378 - Attachment is obsolete: true
Attachment #8760561 - Flags: review?(mconley)
Running this on try (OSX debug 10.10 M-e10s(1) only): https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea2872c9dc65
That build failed due to existing build failure on inbound.  New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0438674018f7
Comment on attachment 8760561 [details] [diff] [review]
bug1272107.patch

The uninit changes look fine to me. Assuming a good try run, r=me.
Attachment #8760561 - Flags: review?(mconley) → review+
Here's a "NOOP" system add-on that should, in theory, do nothing. Let's see if it also breaks the OS X 10.10 DEBUG e10s build :-/
Flags: needinfo?(kvijayan)
Same "NOOP" add-on, this time with "locales".
Attached patch flyweb-ui.patchSplinter Review
Ok.  Turns out that removing locales stuff fixes the problem.  I have no idea why, there seems to be NOTHING off of about the locales usage.  That said, here is a new patch.
Flags: needinfo?(kvijayan)
Attachment #8761317 - Flags: review?(mconley)
Just for refs, the try run for this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e18578afb8d
Attachment #8761317 - Flags: feedback?(francesco.lodolo)
Attachment #8761317 - Flags: feedback?(francesco.lodolo) → feedback?(l10n)
Comment on attachment 8761317 [details] [diff] [review]
flyweb-ui.patch

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

r=me assuming green on try.
Attachment #8761317 - Flags: review?(mconley) → review+
Comment on attachment 8761317 [details] [diff] [review]
flyweb-ui.patch

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

Sorry, but I can't really help on the hang. That'd be really deep in e10s land, mixed maybe with all the unfortunate caching that the stringbundle service does.

Note, to get this code localized, we'll need to have an actual conversation, as basically each system addon does something different.

Please send an email to l10n-drivers@mozilla.org, and describe your expectations?
Attachment #8761317 - Flags: feedback?(l10n)
(In reply to Axel Hecht [:Pike] from comment #20)
> Comment on attachment 8761317 [details] [diff] [review]
> flyweb-ui.patch
> 
> Review of attachment 8761317 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, but I can't really help on the hang. That'd be really deep in e10s
> land, mixed maybe with all the unfortunate caching that the stringbundle
> service does.
> 
> Note, to get this code localized, we'll need to have an actual conversation,
> as basically each system addon does something different.
> 
> Please send an email to l10n-drivers@mozilla.org, and describe your
> expectations?

Hey Pike,

Sorry, we should have been more clear - what we're feedback?ing you for is to see if it's okay to land as-is without localization strings, with the hope of adding those strings later once we sort out this IPC issue.

Does that sound reasonable?
Flags: needinfo?(l10n)
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #21)

> Hey Pike,
> 
> Sorry, we should have been more clear - what we're feedback?ing you for is
> to see if it's okay to land as-is without localization strings, with the
> hope of adding those strings later once we sort out this IPC issue.
> 
> Does that sound reasonable?

Also, to be extra clear, this code is only to land in aurora/nightly and is pref'd off by default.
At least some of our localizers prefer to be able to test their strings in action, and it seems that we'll not have much of a target audience that'd we unleash by adding localized UI to this feature?

In this state, we shouldn't expose it to localizers at all for now, I think.
Flags: needinfo?(l10n)
Cool, thanks for the feedback!
Pushed by kvijayan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc18bf5abac
FlyWeb desktop UI code.  Third time is the charm. r=mconley feedback=Pike
https://hg.mozilla.org/mozilla-central/rev/8dc18bf5abac
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.