Mozilla.ContentTelemetry.initPromise() does not reject if access to HCT isn't granted.

RESOLVED FIXED in Firefox 63

Status

()

defect
P1
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: scolville, Assigned: Dexter)

Tracking

unspecified
mozilla64
Points:
2
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed, firefox64 fixed)

Details

Attachments

(1 attachment)

If you try to test a site without granting access to HCT as per [1] you will see the following in the browser console:

1536671063064	Toolkit.Telemetry	WARN	HybridContentTelemetryListener::handleEvent - accessing telemetry from an untrusted origin.

In addition the promise from Mozilla.ContentTelemetry.initPromise() neither resolves nor rejects. 

If it can't assert the lib is ready I would expect this to reject. Without this you end up with unreachable code after initPromise is called.

[1] https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/hybrid-content.html#testing
:gfristsche would it be possible to prioritise this. If possible I'd prefer to not land the HCT patch in the disco pane before this is fixed, so AFAICT this is the only blocker aside from the data review.
Flags: needinfo?(gfritzsche)
Alessio, can you take a look at what this entails?

Stuart, how soon are you planning to look at this? Is resolving this next week sufficient or does this need a look this week?
Flags: needinfo?(scolville)
Flags: needinfo?(gfritzsche)
Flags: needinfo?(alessio.placitelli)
The patch to put HCT in the disco pane is ready to go otherwise (see https://github.com/mozilla/addons-frontend/pull/5741), so my plan was to land it in master, and enable it on -dev for testing there and the data review. Once the data review is complete it can go to stage and after that production. As for specific timing we're keen to see this land as soon as possible - so if it's possible to fix this sooner that would be great.
Flags: needinfo?(scolville)
(Assignee)

Comment 4

7 months ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #2)
> Alessio, can you take a look at what this entails?

This should probably be fixable by sending a message back to the content at [1], and adding a listener for that error message at [2], which rejects the promise.

This could be a trivial fix, unless I'm forgetting some chrome <-> frame <-> content fun bit.

[1] - https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/toolkit/components/telemetry/hybrid-content/content-HybridContentTelemetry.js#98,102
[2] - https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/toolkit/components/telemetry/hybrid-content/HybridContentTelemetry-lib.js#66
Assignee: nobody → alessio.placitelli
Points: --- → 2
Flags: needinfo?(alessio.placitelli)
Priority: -- → P1
(Assignee)

Comment 5

7 months ago
This patch enables Hybrid Content Telemetry's |initPromise| to reject
if it is disabled or if the host requesting the API doesn't have
enough privileges. This also updates the documentation to mention
the change in behaviour.

Comment 6

7 months ago
Comment on attachment 9009052 [details]
Bug 1490284 - Make Mozilla.ContentTelemetry.initPromise reject. r?chutten

Chris H-C :chutten has approved the revision.
Attachment #9009052 - Flags: review+

Comment 8

7 months ago
Pushed by aplacitelli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ec86654e0c9
Make Mozilla.ContentTelemetry.initPromise reject. r=chutten

Comment 9

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7ec86654e0c9
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(Assignee)

Updated

7 months ago
Depends on: 1491888
(Assignee)

Comment 10

7 months ago
Stuart, do you require uplifts for this?
Flags: needinfo?(scolville)
(In reply to Alessio Placitelli [:Dexter] from comment #10)
> Stuart, do you require uplifts for this?

Does that mean it required Firefox changes as well as an update to just the library?
Flags: needinfo?(scolville) → needinfo?(alessio.placitelli)
I see it does, then I guess the answer for this is yes. We want to be able to make use of this as soon as possible. What version would have support?
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 13

7 months ago
(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #12)
> I see it does, then I guess the answer for this is yes. We want to be able
> to make use of this as soon as possible. What version would have support?

It's in today's nightly (64), will be in 63 (at most).

Would you kindly verify that the updated library solves your problem?
Flags: needinfo?(scolville)
Looks good from testing with nightly and the newer version of the lib.

Alessio - re: the versions this fix will get uplifted to. Will it get back to ESR 60 too?

I'll do some testing, but I think we might be ok to move forward with pushing this on the disco pane even if there are versions out there that won't resolve the promise.
Flags: needinfo?(scolville) → needinfo?(alessio.placitelli)
(Assignee)

Comment 15

7 months ago
(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #14)
> Looks good from testing with nightly and the newer version of the lib.
> 
> Alessio - re: the versions this fix will get uplifted to. Will it get back
> to ESR 60 too?

No, it won't. Do we need to? I'm not sure how uplifting to ESR works, but I bet this doesn't fit the bill for a point release there.
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 16

7 months ago
Comment on attachment 9009052 [details]
Bug 1490284 - Make Mozilla.ContentTelemetry.initPromise reject. r?chutten

Approval Request Comment
[Feature/Bug causing the regression]: hybrid content telemetry
[User impact if declined]: consumer content of the hybrid content telemetry API (e.g. discopane) won't be able to use it to collect data
[Is this code covered by automated tests?]: yes, in this patch
[Has the fix been verified in Nightly?]: yes, see comment 14
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: 1491888
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: this patch makes the initialization promise reject on errors. Previously, the promise was never rejecting. No data collection code has been touched.
[String changes made/needed]: None
Attachment #9009052 - Flags: approval-mozilla-beta?
(In reply to Alessio Placitelli [:Dexter] from comment #15)
> (In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #14)
> > Looks good from testing with nightly and the newer version of the lib.
> > 
> > Alessio - re: the versions this fix will get uplifted to. Will it get back
> > to ESR 60 too?
> 
> No, it won't. Do we need to? I'm not sure how uplifting to ESR works, but I
> bet this doesn't fit the bill for a point release there.

Ok. I'm just wondering if there's some we can fix either in the lib on our side so that the Promise rejects if HCT is not enabled even if the Firefox code isn't there. Maybe using Promise.race() with a second function that runs a timeout with a rejection could ensure that the promise always rejects in lieu of the proper implementation. Not sure if that's something that should be done inside the lib or in our implementation? It feels like a bit of a hack but then leaving an promise unresolved/unrejected on older browsers is not ideal.

What do you think?
Flags: needinfo?(alessio.placitelli)
Comment on attachment 9009052 [details]
Bug 1490284 - Make Mozilla.ContentTelemetry.initPromise reject. r?chutten

P1 telemetry bug, uplift approved for 63 beta 9, thanks.
Attachment #9009052 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 20

7 months ago
(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #17)
> (In reply to Alessio Placitelli [:Dexter] from comment #15)
> > (In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #14)
> > > Looks good from testing with nightly and the newer version of the lib.
> > > 
> > > Alessio - re: the versions this fix will get uplifted to. Will it get back
> > > to ESR 60 too?
> > 
> > No, it won't. Do we need to? I'm not sure how uplifting to ESR works, but I
> > bet this doesn't fit the bill for a point release there.
> 
> Ok. I'm just wondering if there's some we can fix either in the lib on our
> side so that the Promise rejects if HCT is not enabled even if the Firefox
> code isn't there. Maybe using Promise.race() with a second function that
> runs a timeout with a rejection could ensure that the promise always rejects
> in lieu of the proper implementation. Not sure if that's something that
> should be done inside the lib or in our implementation? It feels like a bit
> of a hack but then leaving an promise unresolved/unrejected on older
> browsers is not ideal.
> 
> What do you think?

That could be a good workaround. This sounds hackish for living in the library, but I feel like that's the place it makes most sense: other consumers of the API could have the same problem, and would need the same solution. I'm leaving my ni? around so that I can file a new bug on monday.
(Assignee)

Comment 21

7 months ago
(In reply to Alessio Placitelli [:Dexter] from comment #20)
> (In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #17)
> > (In reply to Alessio Placitelli [:Dexter] from comment #15)
> > > (In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #14)
> > > > Looks good from testing with nightly and the newer version of the lib.
> > > > 
> > > > Alessio - re: the versions this fix will get uplifted to. Will it get back
> > > > to ESR 60 too?
> > > 
> > > No, it won't. Do we need to? I'm not sure how uplifting to ESR works, but I
> > > bet this doesn't fit the bill for a point release there.
> > 
> > Ok. I'm just wondering if there's some we can fix either in the lib on our
> > side so that the Promise rejects if HCT is not enabled even if the Firefox
> > code isn't there. Maybe using Promise.race() with a second function that
> > runs a timeout with a rejection could ensure that the promise always rejects
> > in lieu of the proper implementation. Not sure if that's something that
> > should be done inside the lib or in our implementation? It feels like a bit
> > of a hack but then leaving an promise unresolved/unrejected on older
> > browsers is not ideal.
> > 
> > What do you think?
> 
> That could be a good workaround. This sounds hackish for living in the
> library, but I feel like that's the place it makes most sense: other
> consumers of the API could have the same problem, and would need the same
> solution. I'm leaving my ni? around so that I can file a new bug on monday.

I finally filed bug 1494896.
Blocks: 1494896
Flags: needinfo?(alessio.placitelli)
You need to log in before you can comment on or make changes to this bug.