Closed Bug 1495301 Opened Last year Closed Last year

Perma Multiple wpt failures on /payment-request when Gecko 64 merges to Beta on 2018-10-15

Categories

(Core :: DOM: Web Payments, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- unaffected
firefox64 + verified

People

(Reporter: NarcisB, Assigned: edenchuang)

References

Details

(Whiteboard: [webpayments-reserve])

Attachments

(1 file, 2 obsolete files)

Summary: Multiple wpt failures on /payment-request when Gecko 64 merges to Beta on 2018-10-15 → Perma Multiple wpt failures on /payment-request when Gecko 64 merges to Beta on 2018-10-15
It seems PaymentRequest API is not built in beta environment.

Try to figure it out what wrong with it.
Flags: needinfo?(echuang)
Thanks for looking into this Eden.
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Priority: -- → P2
The root cause is here

https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/dom/payments/PaymentRequest.cpp#60-68

Matt, Marcos, Should PaymentRequest API be used in beta or just keep it in nightly?
Flags: needinfo?(mcaceres)
Flags: needinfo?(MattN+bmo)
Pretty sure it should be Nightly only at the moment? Right Matt?
Flags: needinfo?(mcaceres)
Yeah, Nightly-only because people were trying to enable it beta/Nightly and were getting crashes or reporting issues that we didn't care about. Can we have the WPT tests also check NIGHTLY_BUILD?
Flags: needinfo?(MattN+bmo)
Sebastian, Thanks for the patch. It really helps.

I am going to submit a patch base on the patch and add "disabled" flag for the tests under /feature-policy.
We only expose the Payment-Request on nightly, so disable the web-platform tests on non-nightly build.
Attachment #9013965 - Attachment is obsolete: true
Attachment #9014373 - Flags: review?(amarchesini)
I did the opposite in bug 1496114. Any particular reason why should we enable Payment just in nightly? If the pref is on, Payment API should be enabled everywhere as any other under-development API.
Flags: needinfo?(echuang)
(In reply to Andrea Marchesini [:baku] from comment #12)
> I did the opposite in bug 1496114. Any particular reason why should we
> enable Payment just in nightly? If the pref is on, Payment API should be
> enabled everywhere as any other under-development API.

Did not notice the fix on bug 1496114.
I wrote the patch base on the comment 5 and comment 6.
I am totally fine with removing the nightly build only restriction.
Maybe front-end team would like to do more user testing in nightly for stability and reliability.

Matt, Marcos, could you describe the concerns for turn on PaymentRequest API on Beta by perf dom.payments.request.enabled?
If you think the pref guard is enough for Beta. I will set this bug as dup of bug 1496114.
Flags: needinfo?(mcaceres)
Flags: needinfo?(echuang)
Flags: needinfo?(MattN+bmo)
> Matt, Marcos, could you describe the concerns for turn on PaymentRequest API
> on Beta by perf dom.payments.request.enabled?

Note that we are not enabling payment API in beta. We just enable it for testing when running WPTs.
A) Personally I think we shouldn't even *build* the feature on beta since it's a waste of memory if it's not ready for use. That's what we do with the front-end.
B) As I said in comment 6, users were reporting crashes that we didn't care about so that's why we have the feature check the channel. Since we don't package the front-end outside Nightly the crashes will continue to be a problem unless we change that but changing that is just a distraction from more important things IMO. Triaging the beta/release bug reports will also be wasting more of our time.
Flags: needinfo?(MattN+bmo)
> A) Personally I think we shouldn't even *build* the feature on beta since
> it's a waste of memory if it's not ready for use. That's what we do with the
> front-end.

The current code is fully compiled and part of firefox. Just the PaymentRequest CTOR is not exposed to content if not nightly. Same result of checking the pref, same memory usage.

In platform we build all the features also if disabled by pref. There are several examples of APIs under-developmented disabled by prefs in beta right now. Those APIs are usually written in C++/rust and there is not waste of memory because they are not exposed to content. If the user doesn't touch about:config, of course...

> B) As I said in comment 6, users were reporting crashes that we didn't care
> about so that's why we have the feature check the channel. Since we don't
> package the front-end outside Nightly the crashes will continue to be a
> problem unless we change that but changing that is just a distraction from
> more important things IMO. Triaging the beta/release bug reports will also
> be wasting more of our time.

Yes. I see the problem. touching about:config can trigger several issues.
I'm fine of the  Eden's patch or what I pushed to m-i this morning.

But, is it a real problem the number of crash reports for users in beta who enabled the pref?
(In reply to Andrea Marchesini [:baku] from comment #16)
> > A) Personally I think we shouldn't even *build* the feature on beta since
> > it's a waste of memory if it's not ready for use. That's what we do with the
> > front-end.
> 
> The current code is fully compiled and part of firefox. Just the
> PaymentRequest CTOR is not exposed to content if not nightly. Same result of
> checking the pref, same memory usage.

I know that and it seems wrong, especially for mobile.

> In platform we build all the features also if disabled by pref. There are
> several examples of APIs under-developmented disabled by prefs in beta right
> now. Those APIs are usually written in C++/rust and there is not waste of
> memory because they are not exposed to content. If the user doesn't touch
> about:config, of course...

Doesn't it still take memory from the binary size?

> > B) As I said in comment 6, users were reporting crashes that we didn't care
> > about so that's why we have the feature check the channel. Since we don't
> > package the front-end outside Nightly the crashes will continue to be a
> > problem unless we change that but changing that is just a distraction from
> > more important things IMO. Triaging the beta/release bug reports will also
> > be wasting more of our time.
> 
> Yes. I see the problem. touching about:config can trigger several issues.
> I'm fine of the  Eden's patch or what I pushed to m-i this morning.
> 
> But, is it a real problem the number of crash reports for users in beta who
> enabled the pref?

Yeah, see the bug that added the check. There were at least 3 bugs filed and we got nagged by relman to fix the crashes.
We also got crash complaints on IRC from random people btw.
I don't have a strong opinion, but feel we should follow normal practice: At least on the DOM side, we generally ship APIs in the way Baku mentioned. I'm sympathetic to what MattN is saying about getting bogus reports for things we know are still very much "alpha" quality.
Flags: needinfo?(mcaceres)
QA Contact: mdaly
QA Contact: mdaly
Flags: qe-verify?
Priority: P2 → P1
Whiteboard: [webpayments-reserve]
IMO, For web-platform tests, we should not treat PaymentRequest API different in Beta and Nightly. But we might not want it can be tested for user testing in Beta. 

So I would like to dup this bug as bug 1496114, But adding more enabling control in Beta for reducing the bug report we don't care now.
Duplicate of this bug: 1496114
Attachment #9014373 - Attachment is obsolete: true
Attachment #9014373 - Flags: review?(amarchesini)
Attachment #9016612 - Flags: review+
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ce5fe12ae8d
Disbale payment-request web-platform tests on non-nightly build. r=a=test-only,revert
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8ce5fe12ae8d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: qe-verify? → qe-verify+
See Also: → 1544637
You need to log in before you can comment on or make changes to this bug.