Last Comment Bug 728171 - Make use of Scoped.h
: Make use of Scoped.h
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: David Teller [:Yoric] (please use "needinfo")
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 718938 745291
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-17 02:52 PST by David Teller [:Yoric] (please use "needinfo")
Modified: 2012-04-13 12:12 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Experimental patch (6.53 KB, patch)
2012-02-21 06:31 PST, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Experimental patch (8.56 KB, patch)
2012-02-21 10:37 PST, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Experimental patch (8.59 KB, patch)
2012-02-27 11:08 PST, David Teller [:Yoric] (please use "needinfo")
cjones.bugs: review+
Details | Diff | Splinter Review
Experimental patch (8.60 KB, patch)
2012-03-01 02:27 PST, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Variant, withoutnon-const get() (2.92 KB, patch)
2012-03-01 03:34 PST, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Using Scoped.h throughout the code (14.14 KB, patch)
2012-04-04 09:13 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Using Scoped.h throughout the code (14.96 KB, patch)
2012-04-10 06:20 PDT, David Teller [:Yoric] (please use "needinfo")
cjones.bugs: review+
Details | Diff | Splinter Review
Using Scoped.h throughout the code (281 bytes, patch)
2012-04-11 01:48 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Using Scoped.h throughout the code (185 bytes, patch)
2012-04-11 05:50 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Using Scoped.h throughout the code (14.96 KB, patch)
2012-04-11 05:52 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Using Scoped.h throughout the code (17.76 KB, patch)
2012-04-11 10:00 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Using Scoped.h throughout the code (18.87 KB, patch)
2012-04-12 03:23 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2012-02-17 02:52:25 PST
Bug 718938 introduces mfbt/Scoped.h and a number of general-purpose auto_ptr-style utilities. Let's use them!
Comment 1 David Teller [:Yoric] (please use "needinfo") 2012-02-21 06:31:49 PST
Created attachment 599147 [details] [diff] [review]
Experimental patch

Attaching an experimental patch. Everything compiles, tests in progress.
Comment 2 David Teller [:Yoric] (please use "needinfo") 2012-02-21 10:37:07 PST
Created attachment 599252 [details] [diff] [review]
Experimental patch

Ok, this seems to work.
Comment 3 Mozilla RelEng Bot 2012-02-22 01:45:19 PST
Try run for 872d1fb41e05 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=872d1fb41e05
Results (out of 217 total builds):
    exception: 3
    success: 180
    warnings: 13
    failure: 21
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.com-872d1fb41e05
Comment 4 David Teller [:Yoric] (please use "needinfo") 2012-02-27 11:08:57 PST
Created attachment 600979 [details] [diff] [review]
Experimental patch

Same patch, with slight clean-up in PluginModuleParent.cpp.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-28 11:39:54 PST
Comment on attachment 600979 [details] [diff] [review]
Experimental patch

>diff --git a/xpcom/glue/FileUtils.h b/xpcom/glue/FileUtils.h

>+struct ScopedClosePRFDTraits

>+  static void release(PRFileDesc* fd) {
>+    if (fd) {
>+      PR_Close(fd);
>+    }

This check is unnecessary.

r=me with that fix.
Comment 6 David Teller [:Yoric] (please use "needinfo") 2012-03-01 02:27:40 PST
Created attachment 601907 [details] [diff] [review]
Experimental patch

Attaching a new version. I cleaned up FileUtils.h a little, and I propagating a tiny change from Scoped.h to dom/plugins/ipc/PluginModuleParent.cpp. \n Chris, do you think that we should land this patch once Scoped.h is landed?
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-01 03:15:00 PST
yes

phone
Comment 8 David Teller [:Yoric] (please use "needinfo") 2012-03-01 03:34:56 PST
Created attachment 601918 [details] [diff] [review]
Variant, withoutnon-const get()

Attaching a variant matching a proposed change to Scoped.h without non-|const| |get()| and |operator->|. Which one do you think is best?
Comment 9 David Teller [:Yoric] (please use "needinfo") 2012-04-04 09:13:06 PDT
Created attachment 612221 [details] [diff] [review]
Using Scoped.h throughout the code
Comment 10 David Teller [:Yoric] (please use "needinfo") 2012-04-04 09:14:01 PDT
Updating to reflect the changes in Scoped.h and in the rest of m-c.
Comment 11 David Teller [:Yoric] (please use "needinfo") 2012-04-10 06:20:17 PDT
Created attachment 613570 [details] [diff] [review]
Using Scoped.h throughout the code

Updated to take into account changes in parent bug and to m-c.

Chris, is your r+ sufficient to land this? Otherwise, who should I contact?
Comment 12 David Teller [:Yoric] (please use "needinfo") 2012-04-11 01:48:21 PDT
Created attachment 613910 [details] [diff] [review]
Using Scoped.h throughout the code
Comment 13 David Teller [:Yoric] (please use "needinfo") 2012-04-11 05:50:25 PDT
Created attachment 613960 [details] [diff] [review]
Using Scoped.h throughout the code

Somehow, when I call a socket a "socker", they take offense.
Comment 14 David Teller [:Yoric] (please use "needinfo") 2012-04-11 05:52:09 PDT
Created attachment 613961 [details] [diff] [review]
Using Scoped.h throughout the code
Comment 15 Mozilla RelEng Bot 2012-04-11 07:47:06 PDT
Try run for 6a632ac82fe1 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6a632ac82fe1
Results (out of 221 total builds):
    exception: 1
    success: 187
    warnings: 32
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.com-6a632ac82fe1
Comment 16 David Teller [:Yoric] (please use "needinfo") 2012-04-11 10:00:40 PDT
Created attachment 614045 [details] [diff] [review]
Using Scoped.h throughout the code
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-04-11 15:01:01 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e03eb171e08
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-04-11 15:10:49 PDT
And a bustage fix for poor un-bitrotting on my part.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4058f038e97f
Comment 19 Richard Newman [:rnewman] 2012-04-11 15:35:04 PDT
(In reply to Ryan VanderMeulen from comment #18)
> And a bustage fix for poor un-bitrotting on my part.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4058f038e97f

Backed out per Yoric for bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/084cc1528d4f
Comment 20 Ryan VanderMeulen [:RyanVM] 2012-04-11 19:02:13 PDT
Please post an unbitrotted patch (preferably with a passing Try run).
Comment 21 David Teller [:Yoric] (please use "needinfo") 2012-04-12 03:23:11 PDT
Created attachment 614324 [details] [diff] [review]
Using Scoped.h throughout the code

I have some difficulties understanding what happened. I have just pulled and my patch seems to apply cleanly. Michael Wu pointed another error, tough, so I take the opportunity to fix it.

According to TryServer, that patch builds on B2G. Currently running it on all platforms.
Comment 22 Mozilla RelEng Bot 2012-04-12 08:45:20 PDT
Try run for 6951c5ef44eb is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6951c5ef44eb
Results (out of 15 total builds):
    exception: 2
    success: 11
    warnings: 1
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.com-6951c5ef44eb
Comment 25 David Teller [:Yoric] (please use "needinfo") 2012-04-12 11:10:35 PDT
I have difficulties following: is it in or out?
Comment 26 David Teller [:Yoric] (please use "needinfo") 2012-04-12 11:41:08 PDT
So the patch is actually out.
Comment 27 David Teller [:Yoric] (please use "needinfo") 2012-04-12 11:53:41 PDT
Ok, today, I finally have a merge issue. I suspect that the previous merge issue was only on inbound, not on master.

Launching one (last?) TryServer run.
Comment 28 Mozilla RelEng Bot 2012-04-12 12:00:22 PDT
Try run for fda720573a26 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fda720573a26
Results (out of 2 total builds):
    success: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.com-fda720573a26
Comment 29 Richard Newman [:rnewman] 2012-04-12 12:27:05 PDT
(In reply to David Rajchenbach Teller [:Yoric] from comment #26)
> So the patch is actually out.

To confirm: I backed out 4058f038e97f and 0e03eb171e08, so yes, this was backed out.
Comment 30 Mozilla RelEng Bot 2012-04-12 15:45:23 PDT
Try run for 85d3f2f97ab2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=85d3f2f97ab2
Results (out of 15 total builds):
    success: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.com-85d3f2f97ab2
Comment 31 David Teller [:Yoric] (please use "needinfo") 2012-04-12 15:49:54 PDT
Crossing fingers...
Comment 32 Ryan VanderMeulen [:RyanVM] 2012-04-12 16:20:35 PDT
*fingers crossed* (and landed by itself for easier backouts :P)
https://hg.mozilla.org/integration/mozilla-inbound/rev/db23b2dac69c

Ril.cpp still had bitrot issues, but I made sure to be more careful about it this time.
Comment 33 Marco Bonardo [::mak] 2012-04-13 04:31:45 PDT
https://hg.mozilla.org/mozilla-central/rev/db23b2dac69c

Note You need to log in before you can comment on or make changes to this bug.