+++ This bug was initially created as a clone of Bug #702448 +++ I need to add some utility functions to aid the writing of tests for view partial source.

I forgot to mention that I modified nsWebBrowserFind to stop it leaking. The change doesn't appear to break anything, but I'm not sure about it.

Comment on attachment 598634 [details] [diff] [review] util functions Review of attachment 598634 [details] [diff] [review]: ----------------------------------------------------------------- Can you explain how this fixes the leak? The code looks like the nsIFind object is only created once. Wouldn't this be freed when the destructor is called? How did you test that the memory leak is now fixed? I can't review this, so I've forwarded the review to Benjamin Smedberg since this is in /embedding/. ::: embedding/components/find/src/nsWebBrowserFind.cpp @@ +789,1 @@ > getter_AddRefs(foundRange)); nit: fix the indentation here ::: toolkit/components/viewsource/test/browser/head.js @@ +45,5 @@ > + > +function openDocument(aURI, aCallback) { > + let tab = window.gBrowser.selectedTab = window.gBrowser.addTab(); > + let browser = tab.linkedBrowser; > + browser._tab = tab; Can you explain why the |browser._tab = tab| is needed here? I don't know enough about this. I wouldn't expect something like that to be needed, but I'm sure there is a reason somewhere. Is it because the linkedBrowser doesn't have a reference to it's associated tab?

Comment on attachment 598637 [details] [diff] [review] test for bug 713810 Review of attachment 598637 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. I really like your style of writing the test and the associated utility functions :) Unfortunately I can't do an official review of these patches. I've forwarded the review to Neil Rashbrook since this is in /toolkit/components/.

> > +function openDocument(aURI, aCallback) { > > + let tab = window.gBrowser.selectedTab = window.gBrowser.addTab(); > > + let browser = tab.linkedBrowser; > > + browser._tab = tab; > > Can you explain why the |browser._tab = tab| is needed here? I don't know > enough about this. I wouldn't expect something like that to be needed, but > I'm sure there is a reason somewhere. Is it because the linkedBrowser > doesn't have a reference to it's associated tab? openDocument can easily pass the tab rather than the browser to the callback function, so this isn't needed.

(In reply to Jared Wein [:jaws] from comment #4) > Can you explain how this fixes the leak? The code looks like the nsIFind > object is only created once. Wouldn't this be freed when the destructor is > called? How did you test that the memory leak is now fixed? It is created once, but it never gets cleaned up. I tried putting |mFind = nsnull| in the destructor, but that didn't help (maybe the destructor is never called, which seems odd). In any case we don't want to be holding all the things nsIFind references forever. My test was, run browser_bug713810 in a debug build, see if it complains. > I can't review this, so I've forwarded the review to Benjamin Smedberg since > this is in /embedding/. Yeah, sorry I forgot this bit was in there. (In reply to Dão Gottwald [:dao] from comment #6) > openDocument can easily pass the tab rather than the browser to the callback > function, so this isn't needed. Yeah, good point.

Comment on attachment 599001 [details] [diff] [review] fix for leaky nsWebBrowserFind (ignore the rest) Drive-by nits: >+ if (/^view-source:/.test(event.target.location)) { >+ info("View source window opened: " + event.target.location); >+ viewSourceWindow.removeEventListener("pageshow", pageShowHandler, false); >+ aCallback(viewSourceWindow); Confusing mix of event.target and viewSourceWindow, which are the same... > if (windows.hasMoreElements()) > ok(false, "Found unexpected view source window still open"); ok(!windows.hasMoreElements(), ...) perhaps? > while (windows.hasMoreElements()) > windows.getNext().QueryInterface(Components.interfaces.nsIDOMWindow).close(); Nit: windows have class info, no need to QI them to DOM interfaces. >+ let tab = window.gBrowser.selectedTab = window.gBrowser.addTab(); [I don't see anywhere that you actually need to select the new tab.] >+ window.openUILinkIn(aURI, "current"); Possibly use loadURI directly? >+ window.gBrowser.selectedTab = aTab; >+ window.gBrowser.removeCurrentTab(); You certainly don't need to select the tab to remove it, just removeTab it.

I've split off the JS part, since I should've done that in the first place, and because people keep doing drive-bys on it.

Comment on attachment 599085 [details] [diff] [review] util functions, v3 >+ // Wait for the inner window to load, not viewSourceWindow. Ah, that makes sense now! >+ info("View source window opened: " + event.target.location); Is the location meaningful for view partial source?

(In reply to neil@parkwaycc.co.uk from comment #11) > >+ info("View source window opened: " + event.target.location); > Is the location meaningful for view partial source? Only really for debugging purposes. It could be useful if for some reason the wrong element was getting selected.

Comment on attachment 599085 [details] [diff] [review] util functions, v3 Fair enough.

Comment on attachment 599085 [details] [diff] [review] util functions, v3 >+ closeDocument(aTab); >+function closeDocument(aTab) { >+ window.gBrowser.removeTab(aTab); >+} Is this function really needed?

Comment on attachment 599001 [details] [diff] [review] fix for leaky nsWebBrowserFind (ignore the rest) Are you leaking the nsWebBrowserFind object? If not, I don't see how this patch could actually help any leaks (and if it does, you really need to figure out how... are we holding a reference cycle which is never broken?). In any case, I think the nsWebBrowserFind changes are harmless, since we don't use mFind anywhere else, and the find is synchronous so we don't have to worry about callback/lifetime issues. I did not review the changes in toolkit/components/viewsource/test/browser/head.js since I don't know that code at all.

Comment on attachment 599001 [details] [diff] [review] fix for leaky nsWebBrowserFind (ignore the rest) Meant to mark r+, but only on the nsWebBrowserFind changes.

(In reply to Dão Gottwald [:dao] from comment #14) > >+function closeDocument(aTab) { > >+ window.gBrowser.removeTab(aTab); > >+} > > Is this function really needed? It's not really needed currently, but if a test ever gets written that does something more complicated than selecting a single node (ie. uses openDocument but not openDocumentSelect), it will make the cleaning up tidier. (In reply to Benjamin Smedberg [:bsmedberg] from comment #15) > Are you leaking the nsWebBrowserFind object? If not, I don't see how this > patch could actually help any leaks (and if it does, you really need to > figure out how... are we holding a reference cycle which is never broken?). > In any case, I think the nsWebBrowserFind changes are harmless, since we > don't use mFind anywhere else, and the find is synchronous so we don't have > to worry about callback/lifetime issues. I don't see how it could be leaking either, but I'm more interested in writing tests that pass than fixing the code at the moment.

(In reply to Geoff Lankow (:darktrojan) from comment #17) > It's not really needed currently, but if a test ever gets written that does > something more complicated than selecting a single node (ie. uses > openDocument but not openDocumentSelect), it will make the cleaning up > tidier. closeDocument(tab) isn't really better than gBrowser.removeTab(tab) in any way, be it in head.js or some test. However, couldn't openDocument just use registerCleanupFunction to remove the tab?

Yes, yes it could.

Comment on attachment 599367 [details] [diff] [review] util functions, v4 >+ let tab = window.gBrowser.addTab(aURI); >+ window.gBrowser.removeTab(tab); 'window.' is redundant.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6f3ebce5864 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d2fa77821de https://hg.mozilla.org/integration/mozilla-inbound/rev/b9abdbdbac1b

https://hg.mozilla.org/mozilla-central/rev/a6f3ebce5864 https://hg.mozilla.org/mozilla-central/rev/1d2fa77821de https://hg.mozilla.org/mozilla-central/rev/b9abdbdbac1b