840124 - implement postMessage API for remote report

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect, P1)

Description

[Mike Connor [:mconnor]]

This needs to get finished.

Comment 1

[Mike Connor [:mconnor]]

Attachment: v1 - postMessage most recent JSON payload on load of iframe content

Comment 2

[Mike Connor [:mconnor]]

Comment on attachment 714540 [details] [diff] [review]
v1 - postMessage most recent JSON payload on load of iframe content

Gavin for review, dveditz for sec-review to make sure I"m doing this correctly (I looked at the original patch comments in bug 719484)

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 714540 [details] [diff] [review]
v1 - postMessage most recent JSON payload on load of iframe content

>diff --git a/browser/base/content/abouthealthreport/abouthealth.js b/browser/base/content/abouthealthreport/abouthealth.js

>+function getReportURI() {
>+  return Services.io.newURI(prefs.get("reportUrl"), null, null);
>+}
>+
>+function getIFrame() {
>+  return document.getElementById("remote-report");
>+}

nit: these seem small enough and not called enough that they are not useful as helpers (in the case of getIFrame() you call it twice in the same function inefficiently).

>+function doJSONInjection(data) {
>+  getIFrame().contentWindow.postMessage(JSON.stringify(data), getReportURI().scheme);

postMessage takes a URI, not a scheme, so I imagine you want reportURI.spec (or .prePath, though it shouldn't matter).

Seems like it would be reasonably easy to test this by pointing the report URI to a test-shipped file.

Comment 4

[Mike Connor [:mconnor]]

Attachment: v1.0.1

comments address, minus the bit about testing, since I don't want to go down the rabbit's hole on FHR vs. test infra while we're still sorting that out elsewhere.  That said, it's fairly trivial to test manually in the meantime, I'll attach a test HTML file.

Note on the changes to allow postMessage to any origin if the pref is pointing at a file URI: I did this to facilitate local testing for the webdevs building the feature, as postMessage currently can't limit on file URIs, per MDN.

Note to webdev types working on this, you can use the following snippet to process the received message.

<script>
window.addEventListener("message", receiveMessage, false);
var reportJSON = null;
function receiveMessage(event) {
  reportJSON = JSON.parse(event.data);
}
</script>

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Mike Connor [:mconnor] from comment #4)
> comments address, minus the bit about testing, since I don't want to go down
> the rabbit's hole on FHR vs. test infra while we're still sorting that out
> elsewhere.  That said, it's fairly trivial to test manually in the meantime,
> I'll attach a test HTML file.

Where is it being sorted out? I don't understand why FHR would be particularly complicated to test using our infra.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 715584 [details] [diff] [review]
v1.0.1

>diff --git a/browser/base/content/abouthealthreport/abouthealth.js b/browser/base/content/abouthealthreport/abouthealth.js

> function refreshJSONPayload() {
>   reporter.getLastPayload().then(refreshDataView);
> }
> 
>+function injectJSON() {
>+  reporter.getLastPayload().then(doJSONInjection);
>+}

Seems like maybe you could combine these since they're doing essentially the same thing (also would guarantee that the data we're postMessaging matches exactly the data we're showing in the "raw data" frame - I don't know how often it gets updated).

> function onShowReportClick() {

>+  iframe.addEventListener("load", injectJSON, false);

You probably want to remove this listener after it first fires? If the iframe navigates itself (or fires multiple load events for sub-resources), you don't want to postMessage the data multiple times AIUI.

r=me with those addressed.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(and a test added as discussed on IRC)

Comment 8

[Mike Connor [:mconnor]]

New approach incoming

Comment 9

[Mike Connor [:mconnor]]

Attachment: part 1: make the report fully remote with a small wrapper (v0.9)

Modifies about:healthreport to have 100% remote UI.

Pseudo-API docs and sample HTML are at https://mconnor.etherpad.mozilla.org/aboutHealthReportAPI

To-dos:
* observe pref changes and update the in-content pages
* make the JSON fetch more robust against early init.

Comment 10

[Mike Connor [:mconnor]]

Attachment: part 1: make the report fully remote with a small wrapper (v1.0)

There will be tests for everything before this lands, but I want to get reviews on the approach ASAP, I'll attach them separately tomorrow.  There's a few pieces that are more robust in this approach that will make testing much more efficient and effective, so it should be less of a pain than it was being previously.

https://mconnor.etherpad.mozilla.org/aboutHealthReportAPI describes the API and has sample JS for the webdev folk to use.

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 723812 [details] [diff] [review]
part 1: make the report fully remote with a small wrapper (v1.0)

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

Not granting r+ because of lacking of promised tests.

I'd like to see some more rigor around promise rejection. We could probably just install a new "error" event and send all errors through it. It's probably best to not use the error strings from the rejected promise because they will likely be very low-level and not very user-friendly.

::: browser/base/content/abouthealthreport/abouthealth.js
@@ +23,3 @@
>  
> +
> +var healthReportWrapper = {

r- for using var ;)

@@ +23,5 @@
>  
> +
> +var healthReportWrapper = {
> +  init: function () {
> +    reporter.onInit().then(healthReportWrapper.refreshPayload);

We probably want to install a 2nd handler to then() to handle the case where init fails (it will happen in the wild).

@@ +27,5 @@
> +    reporter.onInit().then(healthReportWrapper.refreshPayload);
> +
> +    let iframe = document.getElementById("remote-report");
> +    iframe.addEventListener("load", healthReportWrapper.initRemotePage, false);
> +    let report = Services.io.newURI(prefs.get("about.reportUrl"), null, null);

If the pref is not a URI this will throw. GIGO?

@@ +29,5 @@
> +    let iframe = document.getElementById("remote-report");
> +    iframe.addEventListener("load", healthReportWrapper.initRemotePage, false);
> +    let report = Services.io.newURI(prefs.get("about.reportUrl"), null, null);
> +    iframe.src = report.spec;
> +    prefs.observe("uploadEnabled", this.updatePrefState, healthReportWrapper);

I /think/ we'll leak this observer.

@@ +52,5 @@
> +    this.injectData("prefs", prefs);
> +  },
> +
> +  refreshPayload: function () {
> +    reporter.collectAndObtainJSONPayload().then(healthReportWrapper.updatePayload);

We should handle the case where the promise is rejected.

Comment 12

[Mike Connor [:mconnor]]

Attachment: v1.1

* var in let in global scope is silly.  but ok.
* if the pref is mangled the UI will break.  I am not worried about what happens in that case, since it should never happen, and if it does we want to know about it immediately.
* the observer should be fine, the pref wrapper implements nsISupportsWeakReference for this reason.
* added error handlers on the promises, and an error handler for the prefs check in case this blows up for some reason.  Please double check this stuff, I'm not 100%

Today got away from me, but tests will be forthcoming before this lands.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 724223 [details] [diff] [review]
v1.1

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

::: browser/base/content/abouthealthreport/abouthealth.js
@@ +23,5 @@
>  
> +
> +let healthReportWrapper = {
> +  init: function () {
> +    reporter.onInit().then(healthReportWrapper.refreshPayload, 

Nit: trailing whitespace

@@ +57,5 @@
> +    }
> +  },
> +
> +  refreshPayload: function () {
> +    reporter.collectAndObtainJSONPayload().then(healthReportWrapper.updatePayload, 

Nit: trailing whitespace

@@ +67,5 @@
> +  },
> +
> +  injectData: function (type, content) {
> +    let report = Services.io.newURI(prefs.get("about.reportUrl"), null, null);
> +    let reportUrl = report.scheme == "file" ? "*" : report.spec;

I'm guessing there's a security restriction you're trying to get around to facilitate local testing? Could you please comment what exactly.

@@ +84,5 @@
> +      case "DisableDataSubmission":
> +        this.onOptOutClick();
> +        break;
> +      case "EnableDataSubmission":
> +        this.onOptInClick();

We don't know exactly how the page is asking for opt in or opt out. So, perhaps its best if the event pass in the reason and we make the names of the local functions generic?

@@ +93,5 @@
> +      case "RequestCurrentPayload":
> +        this.refreshPayload();
> +        break;
> +      default:
> +        Cu.reportError("Unexpected remote command received: " + evt.detail.command + ". Ignoring command.");

Will the Error Console attribute this to about:healthreport or should the message say something?

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 724223 [details] [diff] [review]
v1.1

Basically moves everything external.  Tests coming up in 30s.

Comment 15

[Mike Connor [:mconnor]]

Attachment: v1.1 (tests)

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 725244 [details] [diff] [review]
v1.1 (tests)

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

While this seems to look good, I'm no expert when it comes to testing these types of things. I'd feel much better if you got r+ from a browser peer.

::: browser/base/content/test/browser_aboutHealthReport.js
@@ +15,5 @@
> +                 .getService(Ci.nsISupports)
> +                 .wrappedJSObject
> +                 .policy;
> +        policy.recordHealthReportUploadEnabled(true,
> +                                           "Resetting after tests.");

We should also revert the policy URL, no? Shouldn't we instead clear user preferences?

@@ +37,5 @@
> +{
> +  desc: "Test the postMessage API",
> +  setup: function ()
> +  {
> +    Services.prefs.setCharPref("datareporting.healthreport.about.reportUrl", 

Nit: Trailing whitespace.

@@ +58,5 @@
> +        }
> +      }, false, true);
> +
> +    } catch(e) {
> +      deferred.resolve()

Shouldn't we reject if this occurs?

@@ +68,5 @@
> +{
> +  desc: "Test the remote commands",
> +  setup: function ()
> +  {
> +    Services.prefs.setCharPref("datareporting.healthreport.about.reportUrl", 

Nit: Trailing whitespace.

@@ +85,5 @@
> +    try {
> +      let win = gBrowser.contentWindow;
> +      win.addEventListener("message", function testLoad(e) {
> +        if (!e.data.command)
> +          return;

Nit: Always use braces.

@@ +135,5 @@
> +    }
> +
> +    finish();
> +  });
> +}

It pains me that we don't have a sane test framework for mochitests.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 724223 [details] [diff] [review]
v1.1

>diff --git a/browser/base/content/abouthealthreport/abouthealth.css b/browser/base/content/abouthealthreport/abouthealth.css

>+#remote-report {
>   width: 100%;
>   height: 100%;
>+  border: 0;
>+  display: flex;
> }

Why is display:flex needed?

>diff --git a/browser/base/content/abouthealthreport/abouthealth.js b/browser/base/content/abouthealthreport/abouthealth.js

I didn't review this file. I think any Firefox reviewer could.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 724223 [details] [diff] [review]
v1.1

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

::: browser/base/content/abouthealthreport/abouthealth.css
@@ +11,4 @@
>    width: 100%;
>    height: 100%;
> +  border: 0;
> +  display: flex;

What is the display:flex needed for?

Comment 19

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59ef39cbab73
https://hg.mozilla.org/integration/mozilla-inbound/rev/12e2ef135ea8

Comment 20

[Joe Drew (not getting mail)]

Backed out because it made mochitest browser-chrome time out in the health report test https://hg.mozilla.org/integration/mozilla-inbound/rev/5c46d8839b7f

Comment 21

[Alex Keybl [:akeybl]]

This doesn't block a meta bug, comment 0 isn't very descriptive, and the tracking nom didn't have an associated comment. Why do we need to fix this in FF21?

Comment 22

[Mike Connor [:mconnor]]

Sorry for the confusion (the meta bug is out of date, we'll fix it)

This is the bug that allows the remote report to request the user's data in order to display the report.  The feature will not work without this patch.  We had to significantly refactor the original implementation in order to support the final visual design that came in after the original patch was written.  The patch is entirely contained within about:healthreport and includes tests, so risk outside of FHR should be zero.

Comment 23

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/b90d8a05bd46
https://hg.mozilla.org/mozilla-central/rev/6af57b0f8d36
https://hg.mozilla.org/mozilla-central/rev/2319b61be00d
https://hg.mozilla.org/mozilla-central/rev/5d93b5f1a4d7
https://hg.mozilla.org/mozilla-central/rev/31f1f2a5e1fb

Comment 24

[Mike Connor [:mconnor]]

Attachment: v1.1 (code) for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: FHR will fail to function in final user-facing form.
Testing completed (on m-c, etc.): baking on Nightly since yesterday, in use by webdev people building the web side.
Risk to taking this patch (and alternatives if risky): no risk to non-FHR.
String or IDL/UUID changes made by this patch: Removes no-longer-used strings, adds nothing at all.

Comment 25

[Mike Connor [:mconnor]]

Attachment: v1.1 (tests) for aurora

See previous comment.

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 730857 [details] [diff] [review]
v1.1 (tests) for aurora

This is a blocker for FHR to go live and helps with user facing part needed for Beta 1.

Comment 27

[Mike Connor [:mconnor]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/87fd15173490
http://hg.mozilla.org/releases/mozilla-aurora/rev/c548bbbd86e2