Closed Bug 1468099 Opened 6 years ago Closed 6 years ago

Add a way to check if all the tabs in a process are in background, and whether all those can be throttled

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Throttling check could use TimeoutManager::BudgetThrottlingEnabled. We need to make TabChild a linked list or something. (Somewhat similar TabChild::HasVisibleTabs() is very broken. It was added for Quantum DOM)
Blocks: 1468097
I guess I want to be able to iterate TabGroups, not TabChilds since TabChilds may share the same TabGroup
Attached patch throttable_tabs.diff (obsolete) (deleted) — Splinter Review
I tested this locally. TabGroupList() isn't used by the patch, but I could imagine code like https://searchfox.org/mozilla-central/rev/bf81d741ff5dd11bb364ef21306da599032fd479/toolkit/components/perfmonitoring/PerformanceUtils.cpp#25 should use it. (that code is currently wrong since it may iterate same tabgroup several times.)
Attachment #8985045 - Flags: review?(afarre)
Attachment #8985045 - Flags: review?(afarre) → review+
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7fc2698f9e19 Add a way to check if all the tabs in a process can be throttled, r=farre
Backed out changeset 7fc2698f9e19 (bug 1468099) for gtest failures on a CLOSED TREE Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/354df588746a Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7fc2698f9e19d42e59c3ce640dba0667801fd8f2&selectedJob=183272557 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=183272557&repo=mozilla-inbound&lineNumber=721 INFO - Moving /Users/cltbld/tasks/task_1529022816/build/tests/bin/plugins/gmp-clearkey to /Users/cltbld/tasks/task_1529022816/build/application/Firefox NightlyDebug.app/Contents/Resources 17:36:12 ERROR - shutil error: Destination path '/Users/cltbld/tasks/task_1529022816/build/application/Firefox NightlyDebug.app/Contents/Resources/gmp-clearkey' already exists 17:36:12 INFO - Moving /Users/cltbld/tasks/task_1529022816/build/tests/bin/plugins/gmp-fake to /Users/cltbld/tasks/task_1529022816/build/application/Firefox NightlyDebug.app/Contents/Resources 17:36:12 INFO - Moving /Users/cltbld/tasks/task_1529022816/build/tests/bin/plugins/gmp-fakeopenh264 to /Users/cltbld/tasks/task_1529022816/build/application/Firefox NightlyDebug.app/Contents/Resources 17:36:12 INFO - Moving /Users/cltbld/tasks/task_1529022816/build/tests/gtest/dependentlibs.list.gtest to /Users/cltbld/tasks/task_1529022816/build/application/Firefox NightlyDebug.app/Contents/Resources 17:36:12 INFO - copying tree: /Users/cltbld/tasks/task_1529022816/build/tests/gtest/gtest_bin to /Users/cltbld/tasks/task_1529022816/build/application/Firefox NightlyDebug.app/Contents/MacOS 17:36:12 INFO - mkdir: /Users/cltbld/tasks/task_1529022816/build/application/Firefox NightlyDebug.app/Contents/MacOS/gtest 17:36:12 INFO - copying tree: /Users/cltbld/tasks/task_1529022816/build/tests/gtest/gtest_bin/gtest to /Users/cltbld/tasks/task_1529022816/build/application/Firefox NightlyDebug.app/Contents/MacOS/gtest 17:36:12 INFO - rmtree: /Users/cltbld/tasks/task_1529022816/build/application/Firefox NightlyDebug.app/Contents/MacOS/gtest 17:36:12 INFO - retry: Calling rmtree with args: ('/Users/cltbld/tasks/task_1529022816/build/application/Firefox NightlyDebug.app/Contents/MacOS/gtest',), kwargs: {}, attempt #1 17:36:12 INFO - Running post-action listener: _resource_record_post_action 17:36:12 INFO - [mozharness: 2018-06-15 00:36:12.570973Z] Finished stage-files step (success) 17:36:12 INFO - [mozharness: 2018-06-15 00:36:12.571112Z] Running run-tests step. 17:36:12 INFO - Running pre-action listener: _resource_record_pre_action 17:36:12 INFO - Running pre-action listener: _set_gcov_prefix 17:36:12 INFO - Running main action method: run_tests 17:36:12 INFO - mkdir: /Users/cltbld/tasks/task_1529022816/build/blobber_upload_dir 17:36:12 INFO - #### Running gtest suites
Flags: needinfo?(bugs)
hmm, that means there is some unrelated leak somewhere.
Flags: needinfo?(bugs)
Yeah, our gtest setup is broken. It doesn't detect leaks, and LinkedList implementation happens to assert hard about leaks. I'll change the patch a bit to hack around this issue.
Attached patch throttable_tabs_2.diff (deleted) — Splinter Review
same but not using statically allocated LinkedList to avoid the shutdown assertion in gtest. remote: View your changes here: remote: https://hg.mozilla.org/try/rev/20ab1c2259176b005113156759fc455aa4501258 remote: https://hg.mozilla.org/try/rev/6bfed2badaa331c483c731fe4ae2d4c999341f3e remote: https://hg.mozilla.org/try/rev/8e1ccac26253886ca224fb95c9a612d3ee267c72 remote: https://hg.mozilla.org/try/rev/d7268334b33f28e05eba00aae019839247579fb0 remote: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7268334b33f28e05eba00aae019839247579fb0 remote: recorded changegroup in replication log in 0.113s
Assignee: nobody → bugs
Attachment #8985045 - Attachment is obsolete: true
Attachment #8985577 - Flags: review?(afarre)
Comment on attachment 8985577 [details] [diff] [review] throttable_tabs_2.diff Review of attachment 8985577 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/TabGroup.cpp @@ +23,5 @@ > namespace dom { > > static StaticRefPtr<TabGroup> sChromeTabGroup; > > +LinkedList<TabGroup>* TabGroup::sTabGroups = nullptr; So, how about StaticAutoPtr<LinkedList<TabGroup>> here instead? @@ +69,5 @@ > + > + if (sTabGroups->isEmpty()) { > + delete sTabGroups; > + sTabGroups = nullptr; > + } With StaticAutoPtr the need for deleting sTabGroups goes away. ::: dom/base/TabGroup.h @@ +178,5 @@ > DocGroupMap mDocGroups; > nsTArray<nsPIDOMWindowOuter*> mWindows; > uint32_t mForegroundCount; > + > + static LinkedList<TabGroup>* sTabGroups; See comment about StaticAutoPtr
Flags: needinfo?(bugs)
StaticAutoPtr explicitly deletes LinkedList, and apparently gtests don't care about leaks, so we'd see the same assertion about list not being empty. Need to explicitly delete the list only when the last TabGroup goes away (or fix all the gtest stuff to not leak ever)
Flags: needinfo?(bugs)
Attachment #8985577 - Flags: review?(afarre) → review+
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1950fa531169 Add a way to check if all the tabs in a process can be throttled, r=farre
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: