Closed Bug 1084245 Opened 10 years ago Closed 10 years ago

MozTcpSocket bustage due to exposedProps usage

Categories

(Core :: DOM: Device Interfaces, defect)

Product:
Core
Component:
DOM: Device Interfaces
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla36
Tracking Flags:
Tracking Status
b2g-v2.1 --- unaffected
b2g-v2.2 --- verified

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: regression, smoketest)

Attachments

(1 file)

Introduce a hacky opt-out of the new security checks for MozTCPSocket. v1
(deleted), patch
bzbarsky
: review+
Details | Diff | Splinter Review 
The correct solution here is bug 885982. I'll attach a mock-up of an interim patch.
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)
See also bug 1083991.
Summary: j2me.js MozTcpSocket bustage due to exposedProps usage → MozTcpSocket bustage due to exposedProps usage
Blocks: 1083991
(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.
(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)
Ok, I'll upload an interim fix.
Assignee: nobody → bobbyholley
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)
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 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 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+
Given that this gaia email functionality is apparently critical-but-untested, NIing asutherland to add some tests. ;-)
Flags: needinfo?(bugmail)
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
(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)
Flags: needinfo?(fabrice)
(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)
Not I.
Flags: needinfo?(josh)
(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)
(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.
(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.
(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.
(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.)
(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.)
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.
https://hg.mozilla.org/mozilla-central/rev/5dc1be48edc3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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
Marty, can you verify that this has indeed been fixed?
Flags: needinfo?(mshuman)
Keywords: verifyme
Verified fixed in Flame 2.2 using repro steps from bug 1083991.
IMAP email accounts are able to sign in successfully.
Flags: needinfo?(mshuman)
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?]
status-b2g-v2.2: fixed → verified
Flags: needinfo?(pbylenga)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga)
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.
(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!
(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.
Blocks: 1097928
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.

Attachment

General

Creator:
Created:
Updated:
Size: