Closed
Bug 1084245
Opened 10 years ago
Closed 10 years ago
MozTcpSocket bustage due to exposedProps usage
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
VERIFIED
FIXED
mozilla36
Tracking | Status | |
---|---|---|
b2g-v2.1 | --- | unaffected |
b2g-v2.2 | --- | verified |
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: regression, smoketest)
Attachments
(1 file)
7.47 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•10 years ago
|
||
The correct solution here is bug 885982. I'll attach a mock-up of an interim patch.
Assignee | ||
Comment 2•10 years ago
|
||
Oh I see, this thing uses DOM_OBJECT, which makes it not really suitable for Cu.cloneInto hackery. jdm, can we just land bug 885982?
Flags: needinfo?(josh)
Assignee | ||
Comment 3•10 years ago
|
||
See also bug 1083991.
Summary: j2me.js MozTcpSocket bustage due to exposedProps usage → MozTcpSocket bustage due to exposedProps usage
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #2) > jdm, can we just land bug 885982? Note that if we can't do this, the alternative is just some sort of Cu.optThisGlobalOutOfProperSecurityBehavior() that we invoke in the TcpSocket code and causes us to skip the associated security checks in that case. But that's certainly much yuckier than just switching to WebIDL which we have patches for and are going to do anyway.
Comment 5•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #2) > Oh I see, this thing uses DOM_OBJECT, which makes it not really suitable for > Cu.cloneInto hackery. > > jdm, can we just land bug 885982? I'd love to, but it's gated on the review for bug 916199 which has been sitting in various queues for months.
Flags: needinfo?(josh)
Assignee | ||
Comment 6•10 years ago
|
||
Ok, I'll upload an interim fix.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → bobbyholley
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8506934 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 8•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e63217afbe01 Joshua + Myk - Can you guys verify that this patch fixes the bustage you guys are seeing in your apps?
Flags: needinfo?(myk)
Flags: needinfo?(jmitchell)
Comment 9•10 years ago
|
||
fwiw, it's also totally breaking gaia's email app. If we don't fix that today we'll have to backout your patch bholley.
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #9) > fwiw, it's also totally breaking gaia's email app. If we don't fix that > today we'll have to backout your patch bholley. In that case, would you mind testing the fix attached to this bug?
Flags: needinfo?(fabrice)
Comment 11•10 years ago
|
||
Comment on attachment 8506934 [details] [diff] [review] Introduce a hacky opt-out of the new security checks for MozTCPSocket. v1 r=crying-bloody-tears
Attachment #8506934 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Given that this gaia email functionality is apparently critical-but-untested, NIing asutherland to add some tests. ;-)
Flags: needinfo?(bugmail)
Assignee | ||
Comment 13•10 years ago
|
||
Verified the fix for the gaia email app locally. Let's get this in the tree. https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc1be48edc3
Comment 14•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #8) > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e63217afbe01 > > Joshua + Myk - Can you guys verify that this patch fixes the bustage you > guys are seeing in your apps? Verified. It fixes the bustage in j2me.js.
Flags: needinfo?(myk)
Updated•10 years ago
|
Flags: needinfo?(fabrice)
Comment 15•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #8) > https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e63217afbe01 > > Joshua + Myk - Can you guys verify that this patch fixes the bustage you > guys are seeing in your apps? "these aren't the Josh's your looking for" Redirecting NI
Flags: needinfo?(jmitchell) → needinfo?(josh)
Comment 17•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #12) > Given that this gaia email functionality is apparently > critical-but-untested, NIing asutherland to add some tests. ;-) There is already test coverage of the e-mail app's use of MozTCPSocket in the Gij (Gaia Integration JS) tests, but according to https://bugzilla.mozilla.org/show_bug.cgi?id=1083571#c6 it was hidden at the time of the regressing landing (any may still be hidden now). The main problem is that the TCPSocket tests only establish TCP connections from a chrome-privileged xpcshell test. The mochitests just verify the permissions from content, but don't use the API. This regrettably/problematically means a lack of chrome/content XPConnect coverage.
Flags: needinfo?(bugmail)
Comment 18•10 years ago
|
||
(In reply to Andrew Sutherland [:asuth] from comment #17) > (In reply to Bobby Holley (:bholley) from comment #12) > > Given that this gaia email functionality is apparently > > critical-but-untested, NIing asutherland to add some tests. ;-) > > There is already test coverage of the e-mail app's use of MozTCPSocket in > the Gij (Gaia Integration JS) tests, but according to > https://bugzilla.mozilla.org/show_bug.cgi?id=1083571#c6 it was hidden at the > time of the regressing landing (any may still be hidden now). > > The main problem is that the TCPSocket tests only establish TCP connections > from a chrome-privileged xpcshell test. The mochitests just verify the > permissions from content, but don't use the API. This > regrettably/problematically means a lack of chrome/content XPConnect > coverage. Would it be hard to port those xpcshell to mochitest? We should at least have some basic tests. I've found many bugs that were not reproducible in a privileged environment.
Comment 19•10 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #18) > (In reply to Andrew Sutherland [:asuth] from comment #17) > > (In reply to Bobby Holley (:bholley) from comment #12) > > > Given that this gaia email functionality is apparently > > > critical-but-untested, NIing asutherland to add some tests. ;-) > > > > There is already test coverage of the e-mail app's use of MozTCPSocket in > > the Gij (Gaia Integration JS) tests, but according to > > https://bugzilla.mozilla.org/show_bug.cgi?id=1083571#c6 it was hidden at the > > time of the regressing landing (any may still be hidden now). > > > > The main problem is that the TCPSocket tests only establish TCP connections > > from a chrome-privileged xpcshell test. The mochitests just verify the > > permissions from content, but don't use the API. This > > regrettably/problematically means a lack of chrome/content XPConnect > > coverage. > > Would it be hard to port those xpcshell to mochitest? We should at least > have some basic tests. I've found many bugs that were not reproducible in a > privileged environment. Modifications of the prior art in bug 885982 shouldn't be too difficult.
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Andrew Sutherland [:asuth] from comment #17) > (In reply to Bobby Holley (:bholley) from comment #12) > > Given that this gaia email functionality is apparently > > critical-but-untested, NIing asutherland to add some tests. ;-) > > There is already test coverage of the e-mail app's use of MozTCPSocket in > the Gij (Gaia Integration JS) tests, but according to > https://bugzilla.mozilla.org/show_bug.cgi?id=1083571#c6 it was hidden at the > time of the regressing landing (any may still be hidden now). We fixed all the Gij failures in bug 1083571, and I this fix wasn't one of the necessary ones to get Gij green, so I believe it is still untested. > The main problem is that the TCPSocket tests only establish TCP connections > from a chrome-privileged xpcshell test. The mochitests just verify the > permissions from content, but don't use the API. This > regrettably/problematically means a lack of chrome/content XPConnect > coverage. Yeah, xpcshell-test can verify internal functionality, but content-exposed APIs still need mochitest-style tests.
Comment 22•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #21) > We fixed all the Gij failures in bug 1083571, and I this fix wasn't one of > the necessary ones to get Gij green, so I believe it is still untested. Whoops, yeah, the tests that I was thinking would have caught this as a side-effect doesn't actually assert that sync successfully completed and are tests about having no messages selected, so they don't mind a lack of messages. Bug 975588 is about fixing the email integration tests to be good/fully re-enabled, which will get us that coverage in gaia. (That bug has been partially-completed back-burner for a long time for low bang/buck reasons on regression-detecting ability in the email app itself. Given that we've now seen two serious platform regressions in the past few weeks where platform tests didn't provide sufficient coverage, it's clear it needs to be front-burner, and so we'll ASAP that up.)
Comment 23•10 years ago
|
||
(Er, and noting that we do have back-end email tests that do in fact fail on this (and did fail on the other platform regression), but they're out-of-tree in the gaia-email-libs-and-more repo and won't add anything coverage-wise once bug 975588 is fixed.)
Comment 24•10 years ago
|
||
I started to port and clean up the TCPSocket tests (client and the derived server) to be mochitests; ran into a small wrinkle/learning experience where MDN implied add_task is available to all mochitests (it's not, just browser tests). I expect to be able to finish the cleanup/porting on Monday, but if someone else with more ownership of dom/network wants to take over, I'm happy to leave it to them.
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5dc1be48edc3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 27•10 years ago
|
||
Moving keywords and nomination from duplicated bug 1083991. Setting Status-v2.2 as fixed via Comment 25
blocking-b2g: --- → 2.2?
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.2:
--- → fixed
Keywords: regression,
smoketest
Comment 28•10 years ago
|
||
Marty, can you verify that this has indeed been fixed?
Flags: needinfo?(mshuman)
Keywords: verifyme
Comment 29•10 years ago
|
||
Verified fixed in Flame 2.2 using repro steps from bug 1083991. IMAP email accounts are able to sign in successfully.
Flags: needinfo?(mshuman)
Comment 30•10 years ago
|
||
Environmental Variables: Device: Flame 2.2 Master BuildID: 20141021040206 Gaia: 457a54fc3200b80e4f5e1cd3acaa062309230732 Gecko: 29fbfc1b31aa Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d Version: 36.0a1 (2.2 Master) Firmware: V180 User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: verifyme
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
Comment 31•10 years ago
|
||
I have spun off 1087145 to track the tests issue and have a patch in-progress. Note that preliminary tests indicate that in-process MozTCPSocket may still be broken (or may have been broken for a long time) due to an inability of content to access the data ArrayBuffer.
Assignee | ||
Comment 32•10 years ago
|
||
(In reply to Andrew Sutherland [:asuth] from comment #31) > I have spun off 1087145 to track the tests issue and have a patch > in-progress. \o/ Thanks for doing that. > Note that preliminary tests indicate that in-process > MozTCPSocket may still be broken (or may have been broken for a long time) > due to an inability of content to access the data ArrayBuffer. Thank goodness we're getting tests!
Comment 33•10 years ago
|
||
(In reply to Andrew Sutherland [:asuth] from comment #31) > I have spun off 1087145 to track the tests issue and have a patch > in-progress. Note that preliminary tests indicate that in-process > MozTCPSocket may still be broken (or may have been broken for a long time) > due to an inability of content to access the data ArrayBuffer. Maybe bug 1057556? Another problem that I've seen is that some exceptions aren't correctly propagated if you're in content (bug 1057557 and bug 1057538), they are propagated in a privileged environment.
Comment 34•9 years ago
|
||
Clearing the blocking nom for 2.2? as this is already fixed/verified on that branch per the status flag
blocking-b2g: 2.2? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•