Last Comment Bug 734018 - Don't patch DOMRequestService into Services.jsm
: Don't patch DOMRequestService into Services.jsm
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Philipp von Weitershausen [:philikon]
:
Mentors:
Depends on: 732982
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-08 00:39 PST by Philipp von Weitershausen [:philikon]
Modified: 2012-04-12 10:05 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (32.08 KB, patch)
2012-04-05 17:03 PDT, Philipp von Weitershausen [:philikon]
gavin.sharp: review+
fabrice: review+
Details | Diff | Splinter Review

Description Philipp von Weitershausen [:philikon] 2012-03-08 00:39:30 PST
Patching Services.jsm from the outside is just bad form. Let's move those to Services.jsm proper. (Note also that bug 732982 is going to define the DOMRequestService there.)

Also, while we're making that modification, let's not use two-letter abbreviations for new service definitions. It is entirely not obvious what "Services.rs" means (same goes for Services.tm, Services.ww, etc... I should really file a bug to fix those.)
Comment 1 Philipp von Weitershausen [:philikon] 2012-04-05 17:03:02 PDT
Created attachment 612744 [details] [diff] [review]
v1
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-05 17:15:18 PDT
Comment on attachment 612744 [details] [diff] [review]
v1

r=me on the Services.jsm change (but also add the new entries to the browser_Services.js test).
Comment 3 [:fabrice] Fabrice Desré 2012-04-05 17:30:36 PDT
Comment on attachment 612744 [details] [diff] [review]
v1

Review of attachment 612744 [details] [diff] [review]:
-----------------------------------------------------------------

We must also update https://developer.mozilla.org/en/JavaScript/Code_modules/Services.jsm
Comment 4 Mozilla RelEng Bot 2012-04-06 04:05:18 PDT
Try run for 53d6f1da2815 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=53d6f1da2815
Results (out of 296 total builds):
    exception: 2
    success: 208
    warnings: 86
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/pweitershausen@mozilla.com-53d6f1da2815
Comment 5 Philipp von Weitershausen [:philikon] 2012-04-06 17:50:10 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/26adf71d5c61
Comment 6 Philipp von Weitershausen [:philikon] 2012-04-06 18:43:25 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e46fd6a8cff5

I can't view the try results, and I figured the 2 exceptions were just random oranges. Apparently not. For posterity, here's the inbound push that turned orange: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d7a4dfac3acf
Comment 7 Philipp von Weitershausen [:philikon] 2012-04-06 21:23:53 PDT
So the problems are leaks. I suspect that we're now somehow leaking Services.ppmm and Services.cpmm (at shutdown, I bet). Am I missing some sort of clean up here? These can't be a special case... Or maybe they are?
Comment 8 Philipp von Weitershausen [:philikon] 2012-04-10 18:36:55 PDT
I split the patch up and -- for now -- left out the part where I updated all uses of cpmm/ppmm to use Services.{cpmm|ppmm}, suspecting those changes introduced the leaks. I'll deal with those in a follow-up, I just want to get this bug out the door.

Here's a try build for just (a) not patching the DOMRequestService into Services.jsm and (b) defining cpmm/ppmm in Services.jsm: https://tbpl.mozilla.org/?tree=Try&rev=162558c57a6b
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-04-10 18:58:41 PDT
We probably shouldn't add cpmm/ppmm to Services.jsm if any use of them introduces leaks.
Comment 10 Philipp von Weitershausen [:philikon] 2012-04-10 19:44:16 PDT
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> We probably shouldn't add cpmm/ppmm to Services.jsm if any use of them
> introduces leaks.

That's assuming that there isn't a bug somewhere that causes the leak. But you make a good point, let's be cautious. I'll just commit the "don't patch DOMRequest into Service.jsm" part.
Comment 11 Philipp von Weitershausen [:philikon] 2012-04-10 22:57:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/836cd0fed646
Comment 12 Mozilla RelEng Bot 2012-04-11 00:01:21 PDT
Try run for 162558c57a6b is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=162558c57a6b
Results (out of 290 total builds):
    exception: 3
    success: 254
    warnings: 32
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/pweitershausen@mozilla.com-162558c57a6b

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