Don't patch DOMRequestService into Services.jsm

RESOLVED FIXED in mozilla14

Status

()

Core
General
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: philikon, Assigned: philikon)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.)
Depends on: 732982
(Assignee)

Updated

6 years ago
Assignee: nobody → philipp
(Assignee)

Updated

6 years ago
Component: DOM → General
QA Contact: general → general
Summary: WebApps.js: don't patch Services.jsm → Don't patch DOMRequestService into Services.jsm, define message managers
Created attachment 612744 [details] [diff] [review]
v1
Attachment #612744 - Flags: review?
Attachment #612744 - Flags: feedback?(fabrice)
(Assignee)

Updated

6 years ago
Attachment #612744 - Flags: review?(gavin.sharp)
Attachment #612744 - Flags: review?(fabrice)
Attachment #612744 - Flags: review?
Attachment #612744 - Flags: feedback?(fabrice)
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).
Attachment #612744 - Flags: review?(gavin.sharp) → review+
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
Attachment #612744 - Flags: review?(fabrice) → review+

Comment 4

5 years ago
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/26adf71d5c61
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
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?
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
We probably shouldn't add cpmm/ppmm to Services.jsm if any use of them introduces leaks.
(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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/836cd0fed646
(Assignee)

Updated

5 years ago
Summary: Don't patch DOMRequestService into Services.jsm, define message managers → Don't patch DOMRequestService into Services.jsm

Comment 12

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/836cd0fed646
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.