Closed
Bug 1495301
Opened 6 years ago
Closed 6 years ago
Perma Multiple wpt failures on /payment-request when Gecko 64 merges to Beta on 2018-10-15
Categories
(Core :: DOM: Web Payments, defect, P1)
Core
DOM: Web Payments
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)
5.53 KB,
patch
|
edenchuang
:
review+
|
Details | Diff | Splinter Review |
Central as Beta: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed,busted,exception,retry,runnable&revision=75e5bf1a70f520ee8ef65fb059ca93a4209b85f3&selectedJob=202468379 Failure log e.g.: https://treeherder.mozilla.org/logviewer.html#?job_id=202468379&repo=try&lineNumber=3550 https://treeherder.mozilla.org/logviewer.html#?job_id=202468332&repo=try&lineNumber=21475 Eden, this is a regression from https://bugzilla.mozilla.org/show_bug.cgi?id=1481295
Flags: needinfo?(echuang)
Updated•6 years ago
|
tracking-firefox64:
--- → ?
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
Assignee | ||
Comment 1•6 years ago
|
||
It seems PaymentRequest API is not built in beta environment. Try to figure it out what wrong with it.
Flags: needinfo?(echuang)
Comment 2•6 years ago
|
||
Thanks for looking into this Eden.
Comment hidden (Intermittent Failures Robot) |
Updated•6 years ago
|
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee | ||
Comment 4•6 years ago
|
||
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)
Comment 5•6 years ago
|
||
Pretty sure it should be Nightly only at the moment? Right Matt?
Flags: needinfo?(mcaceres)
Comment 6•6 years ago
|
||
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)
Comment hidden (Intermittent Failures Robot) |
![]() |
||
Comment 8•6 years ago
|
||
PaymentRequest is perma-disabled for non-Nightly builds and ignores the preference dom.payments.request.enabled there, see https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/dom/payments/PaymentRequest.cpp#60-68
Updated•6 years ago
|
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 10•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
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)
Comment 12•6 years ago
|
||
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)
Assignee | ||
Comment 13•6 years ago
|
||
(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)
Comment 14•6 years ago
|
||
> 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.
Comment 15•6 years ago
|
||
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)
Comment 16•6 years ago
|
||
> 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?
Comment 17•6 years ago
|
||
(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.
Comment 18•6 years ago
|
||
We also got crash complaints on IRC from random people btw.
Comment 19•6 years ago
|
||
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)
Updated•6 years ago
|
QA Contact: mdaly
Updated•6 years ago
|
QA Contact: mdaly
Updated•6 years ago
|
Flags: qe-verify?
Priority: P2 → P1
Whiteboard: [webpayments-reserve]
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 21•6 years ago
|
||
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.
Assignee | ||
Comment 23•6 years ago
|
||
Attachment #9014373 -
Attachment is obsolete: true
Attachment #9014373 -
Flags: review?(amarchesini)
Attachment #9016612 -
Flags: review+
Assignee | ||
Comment 24•6 years ago
|
||
try result on nightly https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0b12082c378b45025ea1c78ca3494e544a318c6 try result on Beta https://treeherder.mozilla.org/#/jobs?repo=try&revision=92e348aab59e5ed2a0ffc11e87cd7d5ebe2a67e9
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 25•6 years ago
|
||
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
Comment 26•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8ce5fe12ae8d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
![]() |
||
Comment 27•6 years ago
|
||
Verified fixed with yesterday's beta simulation: https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed,busted,exception,retry,usercancel,runnable&revision=3157d00d6962b95cdc5d13199556e35dd9e6e641
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Flags: qe-verify? → qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•