Closed
Bug 963075
Opened 10 years ago
Closed 9 years ago
browser_pdfjs_[main|views].js leaks until shutdown when run as a standalone directory
Categories
(Firefox :: PDF Viewer, defect, P3)
Firefox
PDF Viewer
Tracking
()
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: jmaher, Assigned: RyanVM)
References
Details
(Whiteboard: [pdfjs-c-integration])
Attachments
(2 files)
1.36 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
698 bytes,
patch
|
RyanVM
:
review+
yury
:
feedback+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Any thoughts on why this wouldn't leak when full tests are run?
Priority: -- → P3
Whiteboard: [pdfjs-c-integration]
Reporter | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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?
Flags: needinfo?(bdahl)
Assignee | ||
Comment 4•10 years ago
|
||
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•10 years ago
|
||
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?
Flags: needinfo?(bdahl)
Assignee | ||
Comment 6•10 years ago
|
||
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
Whiteboard: [pdfjs-c-integration] → [pdfjs-c-integration][test disabled on OSX debug]
Assignee | ||
Comment 7•10 years ago
|
||
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
Assignee | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
||
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.
Severity: normal → major
Flags: needinfo?(bwalker)
Assignee | ||
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4966e5ebd622
Assignee: nobody → ryanvm
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Assignee | ||
Comment 11•10 years ago
|
||
Whoops, left off the leave-open there. That said, it looks like bug 995431 might have an actual fix for this anyway.
Assignee: ryanvm → nobody
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 12•10 years ago
|
||
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
Reporter | ||
Comment 13•10 years ago
|
||
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
Reporter | ||
Comment 14•10 years ago
|
||
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
Assignee | ||
Comment 15•10 years ago
|
||
Yeah, I had to disable the tests on Aurora when I landed all the chunking work there.
Assignee | ||
Comment 16•10 years ago
|
||
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•10 years ago
|
||
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
Flags: needinfo?(bwalker) → needinfo?(gavin.sharp)
Comment 18•10 years ago
|
||
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?
Flags: needinfo?(gavin.sharp)
Comment 19•10 years ago
|
||
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) 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
Reporter | ||
Updated•10 years ago
|
Blocks: run-by-dir
Comment 20•10 years ago
|
||
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?
Flags: needinfo?(bdahl)
Assignee | ||
Comment 21•10 years ago
|
||
Brendan is on PTO until September.
Flags: needinfo?(bdahl) → needinfo?(ydelendik)
Comment 22•10 years ago
|
||
(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.
Flags: needinfo?(ydelendik)
Comment 24•10 years ago
|
||
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.
Assignee | ||
Comment 25•10 years ago
|
||
Bug 1016737 is likely the same problem if that helps.
Reporter | ||
Comment 26•10 years ago
|
||
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?
Flags: needinfo?(ttaubert)
Comment 27•10 years ago
|
||
(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.
Flags: needinfo?(ttaubert)
Reporter | ||
Comment 28•10 years ago
|
||
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 :)
Reporter | ||
Comment 29•10 years ago
|
||
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.
Updated•10 years ago
|
Whiteboard: [pdfjs-c-integration][test disabled on OSX debug] → [pdfjs-c-integration]
Reporter | ||
Comment 30•10 years ago
|
||
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!
Flags: needinfo?(ttaubert)
Comment 31•10 years ago
|
||
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
Flags: needinfo?(ttaubert)
Reporter | ||
Comment 32•10 years ago
|
||
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•10 years ago
|
||
Yeah, looks like I was really successful at moving those leaks to debug builds...
Comment 34•10 years ago
|
||
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•10 years ago
|
||
Attachment #8479837 -
Flags: review?(ted)
Updated•10 years ago
|
Assignee: nobody → ttaubert
Comment 36•10 years ago
|
||
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".
Attachment #8479837 -
Flags: review?(ted) → review+
Comment 37•10 years ago
|
||
(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•10 years ago
|
||
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.
Keywords: leave-open
Assignee | ||
Comment 39•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8f49394f8069
Assignee | ||
Comment 40•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/dab60baef9a1 https://hg.mozilla.org/releases/mozilla-esr31/rev/3d5d36d02133
Reporter | ||
Comment 41•10 years ago
|
||
Attachment #8486416 -
Flags: review?(ydelendik)
Comment 42•10 years ago
|
||
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
Attachment #8486416 -
Flags: review?(ydelendik) → feedback-
Reporter | ||
Comment 43•10 years ago
|
||
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•10 years ago
|
||
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
Attachment #8486416 -
Flags: feedback- → feedback+
Reporter | ||
Comment 45•10 years ago
|
||
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.
Attachment #8486416 -
Flags: review?(ryanvm)
Assignee | ||
Comment 46•10 years ago
|
||
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 :(
Attachment #8486416 -
Flags: review?(ryanvm) → review+
Reporter | ||
Comment 47•10 years ago
|
||
inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/64cf6a7c9d52
Assignee | ||
Comment 48•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/64cf6a7c9d52
Comment 49•10 years ago
|
||
Should we close this for now and file another bug that re-enables these tests after the Promise issue has been fixed?
Reporter | ||
Comment 50•10 years ago
|
||
I am fine either way- leaving this open seems the simplest route though :)
Comment 51•10 years ago
|
||
Ok, wfm. I'll unassign myself then because I'm not the one fixing it anymore :)
Assignee: ttaubert → nobody
Comment hidden (Legacy TBPL/Treeherder Robot) |
Reporter | ||
Updated•10 years ago
|
No longer blocks: 1057512, run-by-dir
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/b41461a95073
Assignee: nobody → ryanvm
Status: REOPENED → RESOLVED
Closed: 10 years ago → 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
You need to log in
before you can comment on or make changes to this bug.
Description
•