Make nsIDOMActivityOptions a Dictionary

RESOLVED FIXED in mozilla31

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mounir, Assigned: Ehsan)

Tracking

({dev-doc-needed})

Trunk
mozilla31
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Comment hidden (empty)
(Reporter)

Updated

5 years ago
Depends on: 903873
(Reporter)

Updated

5 years ago
Assignee: mounir → nobody
(Assignee)

Updated

5 years ago
Blocks: 903873
No longer depends on: 903873
(Assignee)

Comment 1

5 years ago
Created attachment 8391978 [details] [diff] [review]
Make nsIDOMActivityOptions a dictionary
(Assignee)

Updated

5 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 2

5 years ago
Comment on attachment 8391978 [details] [diff] [review]
Make nsIDOMActivityOptions a dictionary

Fabrice, are these tests enough?  Or do I need to run more tests before landing this?  Not sure how good our test coverage is here.
Attachment #8391978 - Flags: review?(bzbarsky)
Flags: needinfo?(fabrice)
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Blocks: 981845
(Assignee)

Comment 3

5 years ago
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #2)
> Comment on attachment 8391978 [details] [diff] [review]
> Make nsIDOMActivityOptions a dictionary
> 
> Fabrice, are these tests enough?  Or do I need to run more tests before
> landing this?  Not sure how good our test coverage is here.

Err, I meant _these_ tests.  :-)

https://tbpl.mozilla.org/?tree=Try&rev=a70dadde4837&showall=1
Ehsan, most of our test coverage for that is Gaia integration tests. Looking at the patch I don't expect issues and we have them on tbpl.
Flags: needinfo?(fabrice)
Comment on attachment 8391978 [details] [diff] [review]
Make nsIDOMActivityOptions a dictionary

>+  // dictionary contains by the request handler.

"contained"?  This is preexisting, but the comment makes no sense to me, either before or after your patch, as it is...

r=me with that fixed.
Attachment #8391978 - Flags: review?(bzbarsky) → review+
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d6c4ee8615f1 for (at least so far, no other b2g test have finished, just four runs of Mac gaia-unit) https://tbpl.mozilla.org/php/getParsedLog.php?id=36312321&tree=Mozilla-Inbound
gaia-unit, gaia-ui-test, they all look the same. Probably completely broke emulator mochitests too, but while the push above you has finished orange your build still hasn't finished to be able to tell yet.
Mochitests were actually below you, so it just remains to be seen whether the gaia-integration was from this or something else.
And gaia-integration is this, https://tbpl.mozilla.org/php/getParsedLog.php?id=36315227&tree=Mozilla-Inbound, so that's the final tally.
(Assignee)

Comment 11

5 years ago
Hmm, this is weird.  At least the Gu tests were green in my try push in comment 3...

Is this how one runs the Gaia integration tests? <https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/Gaia_integration_tests>  There seems to be a bit of context missing from that page...
Running Gu is documented at https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/gaia-ui-tests/Gaia_UI_Tests_Run_Tests (might need more edits as needed to make it clearer, of course).
(Assignee)

Comment 13

5 years ago
Thanks, I'll give that a shot.
(Assignee)

Comment 14

5 years ago
Created attachment 8393022 [details] [diff] [review]
Test fixes interdiff

This seems to fix the tests, pushing to try to confirm.
Attachment #8393022 - Flags: review?(bzbarsky)
Comment on attachment 8393022 [details] [diff] [review]
Test fixes interdiff

r=me
Attachment #8393022 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

5 years ago
The Gu suite on OSX is *still* orange, even though it was green on my try push.  Looking at the log doesn't help at all because the errors don't show up in the log.  I backed out again: https://hg.mozilla.org/integration/mozilla-inbound/rev/5fd15e849857 on the suspicion that this might be at fault again.  If it is, I may give up on this bug at this point, unless someone shows me _a_ way of getting any useful logs from these tests.

Example orange runs:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1f27124a6e08
Zac, can you help Ehsan out here?
I would try to shield Gu from Ehsan's wrath, if and only if there's a bug with the severity blocker on getting stacks from Gu crashes into the log. Otherwise, it's entirely likely that he'll find and point out https://wiki.mozilla.org/Sheriffing/Job_Visibility_Policy#12.29_Crashes_should_produce_valid_crash_stacks.
Flags: needinfo?(zcampbell)
(Assignee)

Comment 21

5 years ago
I don't really know how to proceed here.  Forgetting the issue of the lack of crash stacks, or even js exceptions in the log, my try push in comment 19 which was the exact same thing that I had to back out from central had a green OS X Gu run.  Is it likely that something landed on inbound in that short period that breaks my patch?  Any other theories?

Anyway, I pushed this to try again on top of inbound this time: <https://tbpl.mozilla.org/?tree=Try&rev=86ef8ccad390>
The very first thing I think whenever anyone says "it was fine on try!" is that they needed a clobber. Inconveniently, it doesn't look like b2g desktop builds admit whether or not they were clobbers to know whether you got any, but because it's no skin off my nose if they take a longer time to build, I clobbered them on inbound and retriggered on your push, so if the next one in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c0e333b0f7dc&jobname=b2g.*macosx comes up green...
Oh!  We need to rev the IIDs of the xpidl interfaces that patch is changing.  I bet that's what's going on....

Comment 24

5 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #18)
> Zac, can you help Ehsan out here?

If it weren't a crash I could help but I've had poor luck replicating TBPL crashes locally.

Hopefully Boris is onto something in c#23. 

Otherwise the best I can do is try and replicate it locally using the TBPL build and get a proper crash stack.
Flags: needinfo?(zcampbell)
(Assignee)

Comment 25

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #23)
> Oh!  We need to rev the IIDs of the xpidl interfaces that patch is changing.
> I bet that's what's going on....

So is that a clobber issue or something?  The patch seems to be green every time I push it to try which does clobber builds.
(Assignee)

Comment 26

5 years ago
(In reply to Phil Ringnalda (:philor) from comment #22)
> The very first thing I think whenever anyone says "it was fine on try!" is
> that they needed a clobber. Inconveniently, it doesn't look like b2g desktop
> builds admit whether or not they were clobbers to know whether you got any,
> but because it's no skin off my nose if they take a longer time to build, I
> clobbered them on inbound and retriggered on your push, so if the next one
> in
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c0e333b0f7dc&jobname=b2g.
> *macosx comes up green...

Lo and behold, it is green!
> So is that a clobber issue or something?

Yes.  We apparently update xpts when the IIDs change or something like that (?), so changing an IDL file without changing the iid means the arguments not coming through correctly, which can lead to crashes.

Zac, I think the question is why these crashes aren't ending up with a stack in the tbpl log like they're supposed to.
Flags: needinfo?(zcampbell)

Comment 28

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #27)
> 
> Zac, I think the question is why these crashes aren't ending up with a stack
> in the tbpl log like they're supposed to.

I can't help on that sorry, crash handling in TBPL is the responsibility of the Marionette/A-team.
Flags: needinfo?(zcampbell)
(Assignee)

Comment 29

5 years ago
(In reply to Zac C (:zac) from comment #28)
> (In reply to Boris Zbarsky [:bz] from comment #27)
> > 
> > Zac, I think the question is why these crashes aren't ending up with a stack
> > in the tbpl log like they're supposed to.
> 
> I can't help on that sorry, crash handling in TBPL is the responsibility of
> the Marionette/A-team.

Filed bug 985525.  If it's not in the right component, please move it.  Thanks!
(Assignee)

Comment 30

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #27)
> > So is that a clobber issue or something?
> 
> Yes.  We apparently update xpts when the IIDs change or something like that
> (?), so changing an IDL file without changing the iid means the arguments
> not coming through correctly, which can lead to crashes.

Hmm, yeah, I actually have a vague memory about a bug years ago where we didn't update the xpt when you added a new const value to the interface without touching the uuid or some such.  Kyle, does this ring any bells?
Flags: needinfo?(khuey)
Yeah the XPT packager doesn't make any changes if you don't change the UUID, even for things that don't strictly affect binary compatibility.
Flags: needinfo?(khuey)
Adding Ahal.  Ahal, can you try to see why we're not getting a crash stack here?
Flags: needinfo?(ahalberstadt)
If this is gaia-ui-test on b2g desktop (looks like it is) then this is bug 985403. Patch already uploaded.
Flags: needinfo?(ahalberstadt)
(Assignee)

Comment 35

5 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #34)
> If this is gaia-ui-test on b2g desktop (looks like it is) then this is bug
> 985403. Patch already uploaded.

Yes, thanks!
https://hg.mozilla.org/mozilla-central/rev/1a7c8da31646
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.