Closed Bug 866528 Opened 11 years ago Closed 10 years ago

Make nsIDOMActivityOptions a Dictionary

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla31

People

(Reporter: mounir, Assigned: ehsan.akhgari)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files)

Make nsIDOMActivityOptions a dictionary
16.49 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Test fixes interdiff
1.60 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
      No description provided.
Depends on: 903873
Assignee: mounir → nobody
Blocks: 903873
No longer depends on: 903873
Assignee: nobody → ehsan
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)
Keywords: dev-doc-needed
Blocks: 981845
(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.
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).
Thanks, I'll give that a shot.
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+
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)
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....
(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)
(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.
(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)
(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)
(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!
(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)
(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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: