930839 - B2G SMS: add test case for generating reference workload

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

Originated in bug 806598, we used to generate SMS reference workload by using b2g-desktop.  However, since bug 920551, MOZ_B2G_RIL is only enabled on platforms has RIL support and b2g-desktop is certainly not in the list.  While we still need a way for future regression tests, this issue must be addressed.

The original patch in bug 806598 is outdated and doesn't work with recent Gecko.  This also raise a concern about sync such script with ongoing Gecko development and an active test case is the first thing comes to my mind.  Besides, to fetch that binary sqlite file out, we need the ability to execute adb command during the test, which follows a python Marionette test script may just fit our need here perfectly.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: WIP: add test case for easy SMS reference workload generation

This patch fails to run because of an object wrapper issue:

JavaScript Error: "Permission denied to access object" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 1120}

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #1)
> WIP: add test case for easy SMS reference workload generation
> This patch fails to run because of an object wrapper issue:

With help from bug 836542, we may include MobileMessageDB.jsm and populate a separate SMS db instance without touching nsIRilMobileMessageDatabaseService.  Dunno if that works yet.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Reminding myself to investigate this.

Comment 4

[Julien Wajsberg [:julienw]]

> This also raise a concern about sync such script with ongoing Gecko
> development and an active test case is the first thing comes to my mind. 

Note that it's also a good thing that reference workloads are generated with older versions of gecko because this helps testing our upgrade paths. I really think we should keep having a set of them from 1.1 (eg: the current ones).

But we also need new ones to have samples with newer properties.

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Note that it's also a good thing that reference workloads are generated with
> older versions of gecko because this helps testing our upgrade paths. I
> really think we should keep having a set of them from 1.1 (eg: the current
> ones).
> 
> But we also need new ones to have samples with newer properties.

With bug 836542, we "may" have a chance to generate reference workloads of any schema revision.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #1)
> Created attachment 822090 [details] [diff] [review]
> WIP: add test case for easy SMS reference workload generation
> 
> This patch fails to run because of an object wrapper issue:
> 
> JavaScript Error: "Permission denied to access object" {file:
> "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js"
> line: 1120}

I think I figured this out.  More to come later tonight.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Working(?) test

Attached is a version of this test that does not hit the security error.  It still doesn't finish running ... not sure if that's a separate bug with the test or related to this.  My understanding of what is happening here follows.

We create an object in the test (which runs with content privileges) and stick a bunch of properties on it.  We then pass this object to an XPCOM service that has been wrapped with SpecialPowers to allow us to access it.  XPConnect wraps our object with a wrapper that allows chrome to manipulate the object.  When we get to http://hg.mozilla.org/mozilla-central/annotate/9f8233fcce1d/dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js#l1343 this looks something like:
        Chrome          |            Content
                     wrapper
       aMessage     --------->      received ------> type: 'sms'
                        |                            senders: -------> Array obj
                        |                            ....

The fun part starts inside the XPCOM service.  It adds more properties onto the object (e.g. http://hg.mozilla.org/mozilla-central/annotate/9f8233fcce1d/dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js#l1398)  Primitive types work fine, but objects (such as Arrays) are where this starts to break down.  Because that Array is created by chrome code it lives in the chrome compartment, and when we set it as a property on the object we're going to get a security wrapper.  After executing line 1401 this looks something like:

        Chrome          |            Content
                     wrapper
       aMessage     --------->      received ------> type: 'sms'
                        |                            senders: -------> Array obj
                     wrapper                         read: FILTER_READ_UNREAD
       Array obj    <------------------------------- readIndex:
      (line 1400)

Now we store this in IndexedDB, which tries to structured clone 'aMessage'.  The structured clone code looks at 'aMessage', sees this is a Chrome->Content wrapper, and unwraps it.  It then looks at all the properties of 'received'.  It clones the primitive properties just fine.  But when it gets to 'readIndex' it sees that it has another security wrapper.  But this is a Content->Chrome wrapper, which cannot be unwrapped.  The structured clone code has no memory that it originally started with a chrome object.  So we fail to unwrap 'readIndex' and the error is thrown.

The change I made to the testcase is to use a SpecialPowers API to create an object in the chrome compartment that allows content to access it and set properties on it.  We then set all of our properties on that object.  So the graph now looks like:

        Chrome                                                |     Content 
                                                              |
       aMessage ===  received ------> type: 'sms'          wrapper
                                      senders:            --------> Array obj
                                      read: FILTER_READ_UNREAD|
                                      readIndex: Array obj    |
                                                (line 1400)   |

Now the only privilege boundary we ever cross is Chrome->Content which is allowed.  It feels like the platform should do something smarter here.  No matter what happens we definitely need better debugging tools for this sort of thing ... it took me the better part of a day to figure out what was going on (since I had to debug this on b2g).

Possible outcomes:

1. We declare that the test was wrong, the platform is correct, and we do this privileged createObject thing.
2. We teach structured clone (and other things?) to remember what they started with so that they see that the Content->Chrome edge is really just going back to where we started.  In practice I think that would mean recomputing wrappers relative to the root for the entire object graph before structured cloning.
3. We change the Chrome->Content wrapper here so that properties Chrome adds goes on the wrapper object (not the underlying wrappee) by default, unless you do something like .wrappedJSObject.  Who knows what that will break.  It does seem weird that Chrome -> Content DOM object gets different behavior from Chrome -> Content JS object though.
4. Something I haven't thought of.

Bobby?

Regardless of that I think I unblocked vicamo here.

Comment 8

[Jon Hylands [:jhylands]]

Attachment: SMS/MMS Generation Script

This is the script I am currently using to generate SMS/MMS reference workloads. There is also an addition to the gaia makefile, which I will add in a separate attachment.

Comment 9

[Jon Hylands [:jhylands]]

Attachment: Makefile changes

This is an addition (needs to be pasted in) to the gaia makefile to support generating SMS/MMS messages.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #7)
> 1. We declare that the test was wrong, the platform is correct, and we do
> this privileged createObject thing.

This. SpecialPowers wrappers are really hacky. I almost wish I hadn't made magically work so well, because people try to use them for this kind of thing. Ordinarily, the service is making a safe assumption that it's dealing only with chrome objects here. The SpecialPowers wrappers jump through all sorts of hoops to violate that assumption, and sometimes stuff breaks like this.

I think we should probably try to evangelize other solutions for doing privileged stuff in mochitests, like ochameau's loadChromeScript (see bug 914633).

> 2. We teach structured clone (and other things?) to remember what they
> started with so that they see that the Content->Chrome edge is really just
> going back to where we started.  In practice I think that would mean
> recomputing wrappers relative to the root for the entire object graph before
> structured cloning.

This is plausible, but if there's not a use-case for it outside of hacked-up testing code I don't think we should support it.

> 3. We change the Chrome->Content wrapper here so that properties Chrome adds
> goes on the wrapper object (not the underlying wrappee) by default, unless
> you do something like .wrappedJSObject. Who knows what that will break.

This is only possible if we implement Xray wrappers for pure JS objects. I haven't yet decided whether (and how) to implement expando objects for such things. If we did that, that would solve our problem here.

> It does seem weird that Chrome -> Content DOM object gets different behavior
> from Chrome -> Content JS object though.

Agreed. See bug 914970 comment 0. It's a very hard problem though. :-(

> No matter what happens we definitely need better debugging tools for this sort of thing

I am open to suggestions.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Bobby Holley (:bholley) from comment #10)
> I am open to suggestions.

I think it's also an option: the way I used to pass objects to chrome service is wrong.  So making this depends on bug 836542 and we can have a separate SMS db for each test case.  Bug 836542 wants to extract basic SMS db code into a jsm and leaves other query methods, thread logic in the original file.  So we can import this jsm and instantiate a new SMS database in each test script.  In other words, we don't have to touch nsIRilMobileMessageDatabaseService anymore, and the object unwrapping issue won't even exist.

Comment 12

[Julien Wajsberg [:julienw]]

Sounds very good and sane to me.

Would also help with unit testing all this!

Comment 13

[Jon Hylands [:jhylands]]

Vicamo - what is the status on this? I need at some point to regenerate reference workloads (see bug 928393).

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Jon Hylands [:jhylands] from comment #13)
> Vicamo - what is the status on this? I need at some point to regenerate
> reference workloads (see bug 928393).

Jon, you can still use v1.1 to generate reference workloads.
http://git.mozilla.org/?p=releases/gecko.git;a=shortlog;h=refs/heads/gecko-18

Or for v1.2, that last working commit is:
http://git.mozilla.org/?p=releases/gecko.git;a=shortlog;h=3af3045d402e8103797dd8ae4868f7f19309b346

Or for m-c:
http://git.mozilla.org/?p=releases/gecko.git;a=shortlog;h=8def22b73109087db73e06811db82ec2cdf0d5c5

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: WIP2: add test case for easy SMS reference workload generation

Comment 16

[BMO Automation]

Firefox OS is not being worked on