751086 - Disable toolkit/mozapps/plugins/tests/browser_bug435788.js for leaks

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Bobby Holley (:bholley)]

Mano fixed some added leaks with CPG over in bug 730340. On central we're leaking 22-23 objects, with the leak threshold set to 23. I believe the fuzz is due to bug 751085, which I just filed.

However, it's still occasionally orange on try with his patch. The reason appears to be that we're leaking 2 extra windows in a test that already leaks.

Without CPG:
[toolkit/mozapps/plugins/tests/browser_bug435788.js]
  1 window(s) [url = chrome://mozapps/content/plugins/pluginInstallerWizard.xul]
  1 window(s) [url = about:blank]

With CPG:
[toolkit/mozapps/plugins/tests/browser_bug435788.js]
  2 window(s) [url = chrome://mozapps/content/plugins/pluginInstallerWizard.xul]
  2 window(s) [url = about:blank]

I think it's safe to bump the leak count here.

Comment 1

[Dão Gottwald [:dao]]

(In reply to Bobby Holley (:bholley) from comment #0)
> However, it's still occasionally orange on try with his patch.

How often?

Increasing the limit for intermittent leaks is risky because it means that new persistent leaks can go unnoticed until you hit the intermittent leak again.

Comment 2

[Dão Gottwald [:dao]]

Well, I guess we're not talking about an intermittent leak here, i.e. the browser_bug435788.js change is persistent?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Dão Gottwald [:dao] from comment #2)
> Well, I guess we're not talking about an intermittent leak here, i.e. the
> browser_bug435788.js change is persistent?

It does not always happen, but it happens reliably enough that I don't think transgressing the threshold with something else intermittent is likely to go undetected.

See
https://tbpl.mozilla.org/?tree=Try&rev=2b7d9a8bfa12
and
https://tbpl.mozilla.org/?tree=Try&rev=7d2e75665ce4

Comment 4

[Bobby Holley (:bholley)]

I'd also be fine with disabling the test though, given that it already leaks. Do you think we should do that instead?

Comment 5

[Asaf Romano (gone)]

I prefer disabling the teat and tracking down the leak in a new bug.

By the way, this leak might just be the gPFS global var in the test, which is never cleared.

Comment 6

[Bobby Holley (:bholley)]

Sounds good to me.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Disable toolkit/mozapps/plugins/tests/browser_bug435788.js for leaks. v1

Flagging Mano for review.

Comment 8

[Bobby Holley (:bholley)]

Filed bug 751100 for tracking down the leak.

Comment 9

[Asaf Romano (gone)]

Comment on attachment 620242 [details] [diff] [review]
Disable toolkit/mozapps/plugins/tests/browser_bug435788.js for leaks. v1

>+#browser_bug435788.js has been disabled for leaks.
> ifneq (mobile,$(MOZ_BUILD_APP))
> _BROWSER_FILES = \
>-  browser_bug435788.js \

Comment out instead, and put the comment right above it (and please reference bug 751100 in the comment)

Comment 10

[Dão Gottwald [:dao]]

(In reply to Mano from comment #5)
> By the way, this leak might just be the gPFS global var in the test, which
> is never cleared.

browser-test.js automatically clears each test's scope.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Mano from comment #9)
> >+#browser_bug435788.js has been disabled for leaks.
> > ifneq (mobile,$(MOZ_BUILD_APP))
> > _BROWSER_FILES = \
> >-  browser_bug435788.js \
> 
> Comment out instead, and put the comment right above it (and please
> reference bug 751100 in the comment)

I can't, because it's a Makefile list. But I'll do the warning trick if you want.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Disable toolkit/mozapps/plugins/tests/browser_bug435788.js for leaks. v2 r=Mano

Comment 13

[Asaf Romano (gone)]

Thanks.

Comment 14

[Luke Wagner [:luke]]

https://hg.mozilla.org/mozilla-central/rev/8899c0604bd1