963075 - browser_pdfjs_[main|views].js leaks until shutdown when run as a standalone directory

Status: RESOLVED FIXED

(Firefox :: PDF Viewer, defect, P3)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

on linux and osx 10.6 only browser_pdfjs_views.js leaks.

on osx 10.8 and all windows platforms both browser_pdfjs_views.js and browser_pdfjs_main.js leak.

here is a log for windows 7:
https://tbpl.mozilla.org/php/getParsedLog.php?id=33432706&tree=Try

as far as I can tell, these two tests are the only ones to actually open/render a pdfjs.  Is it possible that this process leaks windows?  we are just doing a removetab to cleanup.

options:
1) fix the leaking problem
2) disable the tests

Comment 1

[Brendan Dahl [:bdahl]]

Any thoughts on why this wouldn't leak when full tests are run?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

it is possible another test cleans up these windows, it could also be that the browser does it on some kind of cleanup schedule which isn't covered by a normal gc/cc and we don't stay open long enough to benefit from it.

Comment 3

[(PTO June 19-July 7) Ryan VanderMeulen]

Brendan, this is one of the bugs currently preventing us from splitting mochitest-bc into chunks across all platforms (and alleviating an extreme pain point in our test infra). Can you please find some time to look into this or suggest someone who can?

Comment 4

[(PTO June 19-July 7) Ryan VanderMeulen]

Note that we're getting to the point where we're going to start disabling tests otherwise as pressure continues to mount to get bug 819963 fully into production.

Comment 5

[Yury Delendik (:yury)]

I was looking into this issue and was not able to replicate this issue when Firefox is built using debug options (on Mac OSX). Only browser_pdfjs_main.js was executed. Not sure how to proceed with troubleshooting in release mode.

Exceptions in release mode:

 0:13.07 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_main.js | leaked until shutdown [nsGlobalWindow #17 http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf]
 0:13.07 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_main.js | leaked until shutdown [nsGlobalWindow #15 http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf]

(In reply to Joel Maher (:jmaher) from comment #2)
> it is possible another test cleans up these windows, it could also be that
> the browser does it on some kind of cleanup schedule which isn't covered by
> a normal gc/cc and we don't stay open long enough to benefit from it.

So they are collected, but not in release builds? Or it is just bad timing. Who else can look at that from non-pdf.js-tests-falure point of view?

Comment 6

[(PTO June 19-July 7) Ryan VanderMeulen]

Ugh, these went away on Cedar and then made their glorious return *after* we enabled all the fancy new mochitest-bc chunking and devtools splitting in production. I'm skipping these tests on OSX debug for now because they were perma-leaking like the log below:
https://tbpl.mozilla.org/php/getParsedLog.php?id=37652210&tree=Fx-Team

https://hg.mozilla.org/mozilla-central/rev/6149db60c6cb

Comment 7

[(PTO June 19-July 7) Ryan VanderMeulen]

And Windows (PGO only?) as well.
https://hg.mozilla.org/integration/fx-team/rev/4966e5ebd622

https://tbpl.mozilla.org/php/getParsedLog.php?id=37677314&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=37680911&tree=Fx-Team
https://tbpl.mozilla.org/php/getParsedLog.php?id=37697763&tree=Fx-Team

Comment 8

[(PTO June 19-July 7) Ryan VanderMeulen]

Adding to the urgency to get this fixed, bug 995431 just got backed out for causing crashes in browser_pdfjs_views.js that would have been missed starting with the next push where it was skipped on Windows.

Comment 9

[Yury Delendik (:yury)]

Bill, apparently everyone thinks this is a PDF.js bug and blocks landing of the next releases. Can we find somebody from the other teams to look at this issue? Without these tests we have no much tests to check basic functionality and integration with Firefox.

Comment 10

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/4966e5ebd622

Comment 11

[(PTO June 19-July 7) Ryan VanderMeulen]

Whoops, left off the leave-open there. That said, it looks like bug 995431 might have an actual fix for this anyway.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

I am still seeing problems with these two tests when running as a standalone directory.  I know these are greener now with the current method of running tests:
https://tbpl.mozilla.org/?tree=Try&rev=119fb18de24b

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

so this pdfjs leaks are showing up on winxp/7/8 opt pretty consistently, here is a log for w7:
https://tbpl.mozilla.org/php/getParsedLog.php?id=38404077&tree=Try

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

I see a whiteboard comment for [test disabled on OSX debug], I don't see that in the current browser.ini file:
http://dxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/test/browser.ini

Comment 15

[(PTO June 19-July 7) Ryan VanderMeulen]

Yeah, I had to disable the tests on Aurora when I landed all the chunking work there.

Comment 16

[(PTO June 19-July 7) Ryan VanderMeulen]

The whiteboard is out of date. They're enabled on trunk because the leak went away with the recent pdf.js update (though my attempt to backport the fix to Aurora didn't make any difference), so I suspect it's the usual "perturb the tests just right..." situation.

Comment 17

[Bill Walker [:bwalker] [@wfwalker]]

Hi Gavin,

Can you suggest who we might consult about these memory leaks? We think this is some kind of problem with our browser integration.

thanks,
-Bill

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Some of the memshrink folks might be able to offer generic advice, but someone who knows the pdf.js code is probably better positioned to chase down possibilities.

It looks like content windows of loaded PDFs are being held alive somehow until shutdown, but in runs where tests continue to run the windows are eventually released. What about PDF.js's behavior might lead to that kind of pattern? Does pdf.js code hold references to content windows that it doesn't clear consistently?

Comment 19

[Yury Delendik (:yury)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #18)

> What about PDF.js's behavior might lead to that kind of
> pattern? Does pdf.js code hold references to content windows that it doesn't
> clear consistently?

There are no globals that might hold a reference to the windows.

I can think only about an actions controller created by StreamConverter [1], which holds references to the window and document it's connected to, and find controller [2], which binds to the find bar events and looses references when the PDF.js window is unloaded [3]. It will be nice to receive the information what object/event actually holds the references.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#907
[2] http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#919
[3] http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#729

Comment 20

[Vaibhav (:vaibhav1994)]

This is still a problem in a recent try push: https://tbpl.mozilla.org/?tree=Try&rev=763734ecdb93 , https://tbpl.mozilla.org/?tree=Try&rev=92b48c2b06a7 , the tests are failing for bc2 jobs.

The goal is to turn these on as --run-by-dir, we don't have too many tests that are problematic(19 total), if we can fix the easy ones now that will help! :)

Brendan, can you figure out why we get these errors while running only the tests in browser/extensions/pdfjs/test directory?

Comment 21

[(PTO June 19-July 7) Ryan VanderMeulen]

Brendan is on PTO until September.

Comment 22

[Yury Delendik (:yury)]

(In reply to Vaibhav from comment #20)

> Brendan, can you figure out why we get these errors while running only the
> tests in browser/extensions/pdfjs/test directory?

Vaibhav,

Brendan and I were not able to determine source of those errors at PDF.js level (see also comment 19). Is there anybody at the firefox/platform teams who can help to identify the source of the leaks? There is also bug 1016737.

Comment 23

[(PTO June 19-July 7) Ryan VanderMeulen]

Maybe Tim can help?

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Do we have any recent failure logs? I tried reproducing locally but that didn't work. Will be quite hard to find the source of the leak without any way to reproduce.

Comment 25

[(PTO June 19-July 7) Ryan VanderMeulen]

Bug 1016737 is likely the same problem if that helps.

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

ttaubert, what is odd is this only leaks on opt, but it is a leak until shutdown which is a bit different than leaking in the test itself.  I am generating new logs.  

Tim, will you be the person to look at this?

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Joel Maher (:jmaher) from comment #26)
> ttaubert, what is odd is this only leaks on opt, but it is a leak until
> shutdown which is a bit different than leaking in the test itself.  I am
> generating new logs.  

Thanks for generating new logs... Are you sure this is leaking on opt only? The leak detector shouldn't even be active/work on opt builds.

> Tim, will you be the person to look at this?

I don't know anything about PDF.JS but a lot about the leak detector... Hmm.

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

ah, it still shows up:
https://tbpl.mozilla.org/?tree=Try&rev=0b5eb184c71d

and the direct log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46550372&tree=Try

leak until shutdown might be a different thing though...always something new to solve :)

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks Tim!  This is a consistent failure as you can see in the try server run.  That is a good thing as it should make it easier to hunt down and fix.

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

Hey Tim, any further thoughts on this problem?  I suspect it falls into a different category that all the wins you got last week.  Please do let folks know if this is not something you can figure out and we can find others!

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Took me a while (days) to get closer to the problem but I might have found a solution that doesn't only apply to PDF.JS but to tests using workers in general. Let's see:

https://tbpl.mozilla.org/?tree=Try&rev=7f0d904a6d90

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

and those pdf tests still like to leak, although preliminary data has it leaking on debug which it hasn't been doing before.  Still waiting on the results from non debug osx+win to determine if this fixes the opt case:)

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

Yeah, looks like I was really successful at moving those leaks to debug builds...

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

Investigating this further, it seems like the test needs to finish just at the right time to leak. The promises used by PDF.JS to render a page keep the window alive and even after the browser is destroyed and the tab is long gone I can still see debug messages printed from when those promises resolve.

This needs to fixed in the platform. The patch I sent to try is still useful, I'll file a separate bug for the Promise issue.

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-963075-Fix-web-workers-from-leaking-by-simulatin.patch

Comment 36

[(not currently active) Ted Mielczarek]

Comment on attachment 8479837 [details] [diff] [review]
0001-Bug-963075-Fix-web-workers-from-leaking-by-simulatin.patch

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

I feel like we're constantly fighting the battle of "what should we be forcing to clean up between tests".

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #36)
> I feel like we're constantly fighting the battle of "what should we be
> forcing to clean up between tests".

It's more about what to clean up when we shut down but I agree. I filed bug 1057386 not too long ago to reverse that. It shouldn't be test suite's responsibility to know about all this.

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/8f49394f8069

Not sure about leaving this open but it isn't actually fixed until bug 1058695 is resolved.

Comment 39

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/8f49394f8069

Comment 40

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dab60baef9a1
https://hg.mozilla.org/releases/mozilla-esr31/rev/3d5d36d02133

Comment 41

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: temporarily disable until we can fix the root cause (1.0)

Comment 42

[Yury Delendik (:yury)]

Comment on attachment 8486416 [details] [diff] [review]
temporarily disable until we can fix the root cause (1.0)

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

I'll be a wrong person for review, but it looks like you are disabling those test for all platforms. See http://hg.mozilla.org/mozilla-central/rev/6149db60c6cb to disable it for Mac OSX only

Comment 43

[Joel Maher ( :jmaher ) (UTC -8)]

Yury, these fail on all platforms in debug mode now, not just OSX.  Please comment if you have more concerns as that appeared to be a misunderstanding from our conversation yesterday.

Comment 44

[Yury Delendik (:yury)]

Comment on attachment 8486416 [details] [diff] [review]
temporarily disable until we can fix the root cause (1.0)

(In reply to Joel Maher (:jmaher) from comment #43)
> Yury, these fail on all platforms in debug mode now, not just OSX.  Please
> comment if you have more concerns as that appeared to be a misunderstanding
> from our conversation yesterday.

I see. Okay, I did not notice it affects all platforms. Sorry about that

Comment 45

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8486416 [details] [diff] [review]
temporarily disable until we can fix the root cause (1.0)

RyanVM- please verify this looks ok, we have discussed this briefly in this bug and in more details in bug 1058695.

Comment 46

[(PTO June 19-July 7) Ryan VanderMeulen]

Comment on attachment 8486416 [details] [diff] [review]
temporarily disable until we can fix the root cause (1.0)

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

LGTM.

Hopefully the DOM team can find cycles to work on the underlying problem soon :(

Comment 47

[Joel Maher ( :jmaher ) (UTC -8)]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64cf6a7c9d52

Comment 48

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/64cf6a7c9d52

Comment 49

[Tim Taubert [:ttaubert] (inactive)]

Should we close this for now and file another bug that re-enables these tests after the Promise issue has been fixed?

Comment 50

[Joel Maher ( :jmaher ) (UTC -8)]

I am fine either way- leaving this open seems the simplest route though :)

Comment 51

[Tim Taubert [:ttaubert] (inactive)]

Ok, wfm. I'll unassign myself then because I'm not the one fixing it anymore :)

Comment 52

[Legacy TBPL/Treeherder Robot]

submit_timestamp: 2014-10-01T11:39:09
log: https://treeherder.mozilla.org/ui/logviewer.html#?repo=fx-team&job_id=795349
repository: fx-team
who: rvandermeulen[at]mozilla[dot]com
machine: tst-linux32-spot-757
buildname: Ubuntu VM 12.04 fx-team pgo test mochitest-browser-chrome-1
revision: 9206bbc5454f

1904 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_main.js | leaked until shutdown [nsGlobalWindow #11815 inner http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf] - expected PASS
1905 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/extensions/pdfjs/test/browser_pdfjs_main.js | leaked until shutdown [nsGlobalWindow #11813 outer  http://example.com/browser/browser/extensions/pdfjs/test/file_pdfjs_test.pdf] - expected PASS

Comment 53

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/b41461a95073

Comment 54

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b41461a95073