Gabriele Svelto [:gsvelto] (PTO) Reporter
	Description • 12 years ago

See bug 835730 comment 2. The list of observers added when initializing nsBrowserGlue.js currently uses strong references and thus require manual removal on shutdown. Changing those to use weak reference will remove most of the code in _dispose() the rest of which should be moved in onPlacesShutdown() or onProfileShutdown().

	Tamara Hills [:thills]
	Comment 1 • 10 years ago

Modified the addObservers to use weak references.  Removed the code from _dispose that should not be needed as we don't need to clean them up manually anymore.

To test, I ran the following try servers:

https://tbpl.mozilla.org/?tree=Try&rev=4766205a9e1a
https://tbpl.mozilla.org/?tree=Try&rev=0fe40fc580eb (large run with failures.  I subsequently re-ran the failed tests and they succeeded).
https://tbpl.mozilla.org/?tree=Try&rev=433fd52cab4b
https://tbpl.mozilla.org/?tree=Try&rev=88851062516b

Additionally, I did some testing on the device (Hamachi):
=========================================================
-Adhoc testing to make sure I can send/receive SMS, make a call, browse, etc.
-Ran the mochitests - I did get some errors here, but got these errors whether it was with our without my change.
-Ran the marionette tests
-Ran the xpcshell tests on the device.  I did get several failures all of the same type; however, I don't think this was something introduced as it happened without my changes as well  

TEST-UNEXPECTED-PASS | /Volumes/development/device-load/B2G/objdir-gecko/dist/test-stage/xpcshell/tests/security/manager/ssl/tests/unit/test_ev_certs.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>
xpcw: cd /data/local/tests/xpcshell/security/manager/ssl/tests/unit
xpcw: xpcshell -r /data/local/tests/xpcshell/c/httpd.manifest -m -s -e const _HTTPD_JS_PATH = "/data/local/tests/xpcshell/c/httpd.js"; -e const _HEAD_JS_PATH = "/data/local/tests/xpcshell/head.js"; -e const _TESTING_MODULES_DIR = "/data/local/tests/xpcshell/m"; -f /data/local/tests/xpcshell/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/data/local/tests/xpcshell/security/manager/ssl/tests/unit/head_psm.js"]; -e const _TAIL_FILES = []; -e const _TEST_FILE = ["test_ev_certs.js"]; -e _execute_test(); quit(0);

	Marco Bonardo [:mak]
	Comment 2 • 10 years ago

Comment on attachment 8436115 [details] [diff] [review]
bug-836789.patch

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

::: browser/components/nsBrowserGlue.js
@@ +414,3 @@
>  #endif
> +    os.addObserver(this, "browser-search-engine-modified", true);
> +    os.addObserver(this, "browser-search-service", true);

while here, would be very nice if you could refactor this code to use an array of topics, something like
[ "prefservice:after-app-defaults",
  ...
  "browser-search-service"
].forEach(topic => Services.obs.addObserver(this, topic, true));

There are also three tracking properties here:
as far as I can tell _isPlacesLockedObserver, _isPlacesInitObserver and _isPlacesShutdownObserver can be completed removed, as well as the code they may be conditioning.

@@ +417,4 @@
>    },
>  
>    // cleanup (called on application shutdown)
>    _dispose: function BG__dispose() {

you should also remove the function and the callpoint since now it's empty.

That means you can also remove the profile-before-change topic/observer since the only thing it does is invoking _dispose.

	Tamara Hills [:thills]
	Comment 3 • 10 years ago

Made some of the refactorizations as per the feedback.  Thank you for the feedback.

	Tamara Hills [:thills]
	Comment 4 • 10 years ago

Here is the link to the try results: https://tbpl.mozilla.org/?tree=Try&rev=8d99a9b3ae51

	Marco Bonardo [:mak]
	Comment 5 • 10 years ago

Comment on attachment 8443496 [details] [diff] [review]
Some additional modification to take care of a scope problem in .foreach.

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

r=me with the following fixed, and a new try run (you should also run tests on debug builds and Mac please, something like try: -b do -p linux,linux64,macosx64,win32 -u all -t none)

::: browser/components/nsBrowserGlue.js
@@ +370,5 @@
>      let os = Services.obs;
> +    var obsThis = this;
> +    ["prefservice:after-app-defaults",
> +    "final-ui-startup",
> +    "browser-delayed-startup-finished",

please reindent as
[ "topic",
  "topic",
...
  "topic"
].forEach(...

@@ +396,4 @@
>  #endif
> +    "browser-search-engine-modified",
> +    "browser-search-service"].forEach(function observe(topic) {
> +          os.addObserver(obsThis, topic, true);

instead of using the obsThis trick, you can rather use an arrow function here. Arrow functions are automatically bound to "this" defined when they are created

something like 
forEach(topic => {
  os.addObserver(this, topic, true);
})
should work

	Marco Bonardo [:mak]
	Comment 6 • 10 years ago

Hi, are you still working on this bug?

	Tamara Hills [:thills]
	Comment 7 • 10 years ago

Hi Marco,

Sorry for the lag on this.  I will try and finish this off by the end of this sprint (ending on 9/12).  

If it's urgent, let me know and I'll prioritize it.

Thanks,
-tamara

	Marco Bonardo [:mak]
	Comment 8 • 10 years ago

no, it's not urgent, I just wanted to know if it was still in your plans. thanks.

	albert.calzaretto
	Comment 9 • 9 years ago

Hi, I would like to attempt this bug as a good first bug. University of Toronto, CSC302H1S-2015	
assignment. Patch by Feb 26,2016.

	Josh Matthews [:jdm]
	Comment 10 • 9 years ago

That seems like a reasonable choice, Albert. You should be able to use the existing patch as inspiration, making sure to address the requests in the review in comment 5.

	Gabriele Svelto [:gsvelto] (PTO) Reporter
	Comment 11 • 9 years ago

This bug is now assigned to you Albert. Be sure to check that the code didn't change significantly since the bug was filed because this happened a while ago. Feel free to contact me on IRC or via e-mail for help. Happy hacking!

	albert.calzaretto
	Comment 12 • 9 years ago

Thank you for your support!  I appreciate it!

	albert.calzaretto
	Comment 13 • 9 years ago

Alright, so as of now, running the tests, before I removed _dispose ./mach test was running fine 

And here is the patch file before I removed _dispose

>@@ -598,10 +598,6 @@ BrowserGlue.prototype = {
>      os.addObserver(this, topic, true);
>    })
> 
>-    [
>-    ].forEach(topic => {
>-      os.addObserver(this, topic, true);
>-    })
> 
>     ExtensionManagement.registerScript("chrome://browser/content/ext-utils.js");
>     ExtensionManagement.registerScript("chrome://browser/content/ext-browserAction.js");
>@@ -626,44 +622,10 @@ BrowserGlue.prototype = {
>   // cleanup (called on application shutdown)
>   _dispose: function BG__dispose() {
>     let os = Services.obs;
>-    os.removeObserver(this, "notifications-open-settings");
>-    os.removeObserver(this, "prefservice:after-app-defaults");
>-    os.removeObserver(this, "final-ui-startup");
>-    os.removeObserver(this, "sessionstore-windows-restored");
>-    os.removeObserver(this, "browser:purge-session-history");
>-    os.removeObserver(this, "quit-application-requested");
>-    os.removeObserver(this, "quit-application-granted");
>-    os.removeObserver(this, "restart-in-safe-mode");
>-#ifdef OBSERVE_LASTWINDOW_CLOSE_TOPICS
>-    os.removeObserver(this, "browser-lastwindow-close-requested");
>-    os.removeObserver(this, "browser-lastwindow-close-granted");
>-#endif
>-    os.removeObserver(this, "weave:service:ready");
>-    os.removeObserver(this, "weave:engine:clients:display-uri");
>-    os.removeObserver(this, "session-save");
>     if (this._bookmarksBackupIdleTime) {
>       this._idleService.removeIdleObserver(this, this._bookmarksBackupIdleTime);
>       delete this._bookmarksBackupIdleTime;
>-    }
>-
>-    if (this._isPlacesShutdownObserver)
>-      os.removeObserver(this, "places-shutdown");
>-    os.removeObserver(this, "handle-xul-text-link");
>-    os.removeObserver(this, "profile-before-change");
>-#ifdef MOZ_SERVICES_HEALTHREPORT
>-    os.removeObserver(this, "keyword-search");
>-#endif
>-    os.removeObserver(this, "browser-search-engine-modified");
>-    try {
>-      os.removeObserver(this, "browser-search-service");
>-      // may have already been removed by the observer
>-    } catch (ex) {}
>-#ifdef NIGHTLY_BUILD
>-    Services.prefs.removeObserver(POLARIS_ENABLED, this);
>-#endif
>-    os.removeObserver(this, "flash-plugin-hang");
>-    os.removeObserver(this, "xpi-signature-changed");
>-    os.removeObserver(this, "autocomplete-did-enter-text");
>+    }  
>   }

And here is the patch after I removed it: 

> @@ -398,12 +398,6 @@ BrowserGlue.prototype = {
>           }
>         }
>         break;
>-      case "profile-before-change":
>-         // Any component depending on Places should be finalized in
>-         // _onPlacesShutdown.  Any component that doesn't need to act after
>-         // the UI has gone should be finalized in _onQuitApplicationGranted.
>-        this._dispose();
>-        break;
>       case "keyword-search":
>         // This notification is broadcast by the docshell when it "fixes up" a
>         // URI that it's been asked to load into a keyword search.
>@@ -618,16 +612,7 @@ BrowserGlue.prototype = {
>     this._flashHangCount = 0;
>     this._firstWindowReady = new Promise(resolve => this._firstWindowLoaded = resolve);
>   },
>-
>-  // cleanup (called on application shutdown)
>-  _dispose: function BG__dispose() {
>-    let os = Services.obs;
>-    if (this._bookmarksBackupIdleTime) {
>-      this._idleService.removeIdleObserver(this, this._bookmarksBackupIdleTime);
>-      delete this._bookmarksBackupIdleTime;
>-    }  
>-  },
>-
>+  
>   _onAppDefaults: function BG__onAppDefaults() {
>     // apply distribution customizations (prefs)
>     // other customizations are applied in _finalUIStartup()
>@@ -1791,13 +1776,12 @@ BrowserGlue.prototype = {
>    */
>   _onPlacesShutdown: function BG__onPlacesShutdown() {
>     PageThumbs.uninit();
>-
>     if (this._bookmarksBackupIdleTime) {
>       this._idleService.removeIdleObserver(this, this._bookmarksBackupIdleTime);
>       delete this._bookmarksBackupIdleTime;
>     }
>   },

The tests pass in the former patch, but not the latter when running ./mach test.  Any ideas?

	Gabriele Svelto [:gsvelto] (PTO) Reporter
	Comment 14 • 9 years ago

Albert, could you please attach your patch to this bug so I can inspect it? See the documentation here on how to  do it:

https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

	albert.calzaretto
	Comment 15 • 9 years ago

Alright here is the attachment of the patch.  I used git, so I had some issues before, but I think I did this correctly.  It breaks after I removed _dispose.

	Gabriele Svelto [:gsvelto] (PTO) Reporter
	Comment 16 • 9 years ago

Comment on attachment 8722675 [details] [diff] [review]
bug836789.patch

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

You have attached two diffs stitched together, one of which is the patch. I suggest you commit your patch locally and use `git format-patch` to create a proper patch. You can always modify your commit later.

As for the issue with the tests you've encountered it's caused by a syntax error in nsBrowserGlue.js:

::: browser/components/nsBrowserGlue.js
@@ +253,3 @@
>    _isPlacesDatabaseLocked: false,
>    _migrationImportsDefaultBookmarks: false,
> +  _disposed: false

You're missing a comma here; when hitting this the BrowserGlue object creation fails and the tests refuse to run.

With this small issue fixed the tests seem to run correctly. However besides correcting this you'll have to rebase your patch since it doesn't apply to the current master. First fix your patch and ensure that it works correctly and the tests pass, then update your mozilla-central checkout and rebase your patch on top of the current master.

	albert.calzaretto
	Comment 17 • 9 years ago

I had to remove 
>>delete this._distributionCustomizer;
on line 374 because it kept throwing an error when I closed the browser after running it and in all honesty I am not sure whether or not the weak references played a role in it or not. The tests passed, and closing the browser doesnt cause any errors anymore.

	Gabriele Svelto [:gsvelto] (PTO) Reporter
	Comment 18 • 9 years ago

Comment on attachment 8723623 [details] [diff] [review]
Another patch that seems to be working.

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

Below you'll find my feedback on this patch. Just a couple of important notes though: the code in this file has changed somewhat (the #ifdef guards have been removed) so you'll need to rebase your patch on top of the current mozilla-central tree. Please do that before we move to the next iteration.

::: browser/components/nsBrowserGlue.js
@@ +290,4 @@
>  
>    // nsIObserver implementation
>    observe: function BG_observe(subject, topic, data) {
> +    //observers have implcititly disposed

nit: A better comment here would be something explaining why we're doing this, for example

"If the object was disposed then we don't want to respond to events anymore"

@@ -351,5 @@
> -        Services.obs.removeObserver(this, "places-init-complete");
> -        this._isPlacesInitObserver = false;
> -        // no longer needed, since history was initialized completely.
> -        Services.obs.removeObserver(this, "places-database-locked");
> -        this._isPlacesLockedObserver = false;

I don't think we should remove this pieces of code. Since this observer (and the ones below) are not needed anymore when we reach this point we should still remove them.

@@ -358,5 @@
>          this._isPlacesDatabaseLocked = true;
> -        // Stop observing, so further attempts to load history service
> -        // will not show the prompt.
> -        Services.obs.removeObserver(this, "places-database-locked");
> -        this._isPlacesLockedObserver = false;

Likewise

@@ -364,5 @@
>        case "places-shutdown":
> -        if (this._isPlacesShutdownObserver) {
> -          Services.obs.removeObserver(this, "places-shutdown");
> -          this._isPlacesShutdownObserver = false;
> -        }

Likewise

@@ -374,5 @@
>          break;
>        case "distribution-customization-complete":
>          Services.obs.removeObserver(this, "distribution-customization-complete");
> -        // Customization has finished, we don't need the customizer anymore.
> -        delete this._distributionCustomizer;

Likewise (note that here you deleted the field but not removed the observer anyway).

@@ +412,5 @@
> +
> +#ifdef NIGHTLY_BUILD
> +        Services.prefs.removeObserver(POLARIS_ENABLED, this);
> +#endif
> +        this._disposed = true;

I think it would be better to leave the dispose() method around and put this code in it. It makes the intent of the code clearer.

	albert.calzaretto
	Comment 19 • 9 years ago

I will still be working on this bug, however I am going to pull from the centralized branch and not my course branch which isn't up to date which would require another full rebuild. Due to midterms this is low priority as of now, but I plan on finishing it by the end of the month (And it shouldn't take long, it is a trivial bug, its the code refactoring that backfired)

	albert.calzaretto
	Comment 20 • 9 years ago

I will still be working on this bug, however I am going to pull from the centralized branch and not my course branch which isn't up to date which would require another full rebuild. Due to midterms this is low priority as of now, but I plan on finishing it by the end of the month (And it shouldn't take long, it is a trivial bug, its the code refactoring that backfired)

	Gabriele Svelto [:gsvelto] (PTO) Reporter
	Comment 21 • 9 years ago

Take your time, there's no deadline on this, that's why it's a mentored bug :)

	albert.calzaretto
	Comment 22 • 8 years ago

I decided to keep dispose, and not create the new variable _disposed as it did not seem to help refactor the code in any way. I kept it simple this time without major refactoring, just the removal of the 'removeObservers' on the weak reference'd objects.

	Gabriele Svelto [:gsvelto] (PTO) Reporter
	Comment 23 • 8 years ago

Comment on attachment 8735889 [details] [diff] [review]
bug836789.patch

This is looking very good! My only worry is about the removal of the "disposed" field; the reason we added it in the first place is that when using weak references the observers will not be removed immediately after calling the _dispose() method and so the observe() method could still be called afterwards. To preserve the code semantics we'd set the disposed field to true in _dispose() and checked it in observe() before doing anything else. In case disposed was set observe() would bail out immediately instead of running the observers.

Anyway, it might be that this change isn't needed though, and the only way to figure it out is by running tests which is the next step before review. I suggest that you start by running mochitests locally. Mochitests cover a lot of functionality and should be a good starting point to ensure that your patch works correctly, see the guide here on how to run them:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mochitest

Just a word of warning, running *all* mochitests will take a while so I suggest focusing on the ones in the subcomponent you're working on. This can be achieved by passing the appropriate directory to mach, e.g.:

./mach mochitest browser/components

Once you've ensured that the tests run locally with your patch applied it will be time to try our automated test infrastructure, you can read more about it here:

https://wiki.mozilla.org/ReleaseEngineering/TryServer

Ping me on IRC about that so I can walk you through it.

	albert.calzaretto
	Comment 24 • 8 years ago

I ran ./mach test and ./mach mochitest browser/components with everything passing and running smoothly.

	Gabriele Svelto [:gsvelto] (PTO) Reporter
	Comment 25 • 8 years ago

Comment on attachment 8737792 [details] [diff] [review]
bug836789.patch

Good work Albert, I'm not a peer of this component so I can't handle the review myself but I'll redirect it Marco who can review this. You might need to address his comments too but I'm always available for walking you through it; my tutoring is over only when the patch lands :)

I've also launched a fairly extensive try run here to check if everything is OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a52b848ebc0

	Marco Bonardo [:mak]
	Comment 26 • 8 years ago

Comment on attachment 8737792 [details] [diff] [review]
bug836789.patch

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

it's ok, but I'd like to merge all the loops

::: browser/components/nsBrowserGlue.js
@@ +224,1 @@
>    _isPlacesShutdownObserver: false,

this can be removed

@@ +262,4 @@
>  
>    // nsIObserver implementation
>    observe: function BG_observe(subject, topic, data) {
> +    //If the objects were disposed then we don't want to respond to events anymore

Per coding style, please add a space after // and end comments with a period.

@@ +336,3 @@
>          break;
>        case "places-shutdown":
>          if (this._isPlacesShutdownObserver) {

as well as this if condition. The only reason to have it was so that we wouldn't removeObserver on dispose if we already removed the observer here... Since we don't do the former anymore we shouldn't need to check.

@@ +523,4 @@
>      if (OBSERVE_LASTWINDOW_CLOSE_TOPICS) {
> +      os.addObserver(this, "browser-lastwindow-close-requested", true);
> +      os.addObserver(this, "browser-lastwindow-close-granted", true);
> +    }

just move this before/after the other topics, so you can merge the arrays and havea single forEach.
There should be no reason the order here matters that much, it's just the order they were added to the file...

@@ +540,1 @@
>      this._isPlacesShutdownObserver = true;

you can just remove this and merge more topics

@@ +544,3 @@
>      if (AppConstants.MOZ_TELEMETRY_REPORTING) {
> +      os.addObserver(this, "keyword-search", true);
> +    }

just move this after the foreach, and merge further topics

	Pulsebot
	Comment 29 • 3 years ago

Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/a15cb9719169
changed to using weak references to observers in the _init function and deleted unnessesary removeObserver calls in the _dispose function. r=emilio,mak Cobos Álvarez