1358475 - Opening Preferences with subcategory on an existing Preferences page would result blank page

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Fischer [:Fischer]]

STR:
1. Launch Firefox Nightly
2. Open Browser Toolbox > Console
3. Execute `openPreferences("privacy-reports")` in the console
4. Make sure Privacy & Security > checkbox of sending crash reports is displayed
5. Execute `openPreferences("privacy-reports")` in the console again

Expected: Still see Privacy & Security > checkbox of sending crash reports

Actual: A blank page with the title of General only

Comment 1

[Fischer [:Fischer]]

The root cause for this issue is because:
  1. For the 1st time of opening preference with "privacy-reports", a new about:preferences tab would open.

  2. In the 2ndd time of opening preference with "privacy-reports", because already there is a about:preferences tab. The `openPreferences` utility function would try to reuse the existing about:preferences tab by calling `switchToTabHavingURI`[1].
    This is going to load about:preferences#privacy-reports

  3. Then, in the openPreferences` utility function, it will continue proceeding to call `gotoPref("privacy-reports")`[2]

  4. But in the `gotoPref`, the subcategory is extracted from hash[3], which at this moment is "#privacy-reports" so subcategory = "reports" now. And the category = privacy-reports now[4].

  5. Because privacy-reports is an unknown category so unable to get any item from `querySelector`[5]. As a result, it falls back to the default general category.

  6. Then, end up in searching with categroy = general and subcategory = reports so all elements are unable to match this search criteria. So all are hidden.

  7. Although in the step 2 a hash change event would occur, it is behind all the operations above. By the time another `gotoPref` is run, we have already switch to about:preference#general. So an early return happens[6].


[1] https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/browser/base/content/utilityOverlay.js#756
[2] https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/browser/base/content/utilityOverlay.js#773
[3] https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/browser/components/preferences/in-content/preferences.js#154
[4] https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/browser/components/preferences/in-content/preferences.js#158
[5] https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/browser/components/preferences/in-content/preferences.js#166
[6] https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/browser/components/preferences/in-content/preferences.js#163

Comment 2

[Fischer [:Fischer]]

Attachment: Bug 1358475 - The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory.

Review commit: https://reviewboard.mozilla.org/r/132420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132420/

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Good investigation. Please add this to browser_bug1020245_openPreferences_to_paneContent.js before the last `  yield BrowserTestUtils.removeTab(gBrowser.selectedTab);`:

>   openPreferences("privacy-reports");
>   is(doc.location.hash, "#privacy", "The category should change to privacy");
>   ok(!doc.querySelector("#header-privacy").hidden, "The privacy header should be visible for the subcategory");

Comment 5

[Fischer [:Fischer]]

Comment on attachment 8860408 [details]
Bug 1358475 - The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132420/diff/1-2/

Comment 6

[Fischer [:Fischer]]

Comment on attachment 8860408 [details]
Bug 1358475 - The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132420/diff/2-3/

Comment 7

[Fischer [:Fischer]]

Comment on attachment 8860408 [details]
Bug 1358475 - The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory.

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #4)
> Good investigation. Please add this to 
> browser_bug1020245_openPreferences_to_paneContent.js before the last ` 
> yield BrowserTestUtils.removeTab(gBrowser.selectedTab);`:
Hi Jaws,
Adding one new test for this case is because
  - the test goal of the existing one is different from here
  - the handling of this test here is slightly different too

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a81c52b6f2d0885555724c49f9ccfde8c092f4b7&selectedJob=93526612

Thanks

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8860408 [details]
Bug 1358475 - The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory.

https://reviewboard.mozilla.org/r/132420/#review135764

r=me with the following two changes.

::: commit-message-8b854:1
(Diff revision 3)
> +Bug 1358475 - Opening Preferences with subcategory on an existing Preferences page would result blank page

This commit message explains the bug that is being fixed, but doesn't explain why the new code is better than the old code. Please re-write your commit message.

Maybe use:
"Bug 1358475 - The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory. r=jaws

Computing the subcategory based off of just the hash doesn't work when switchToTabHavingURI is used to open the preferences."

::: browser/components/preferences/in-content/tests/browser_bug1020245_openPreferences_to_paneContent.js:42
(Diff revision 3)
>    yield BrowserTestUtils.removeTab(gBrowser.selectedTab);
>  });
>  
> +// Test opening Preferences with subcategory on an existing Preferences tab. See bug 1358475.
> +add_task(function*() {
> +  let prefs = yield openPreferencesViaOpenPreferencesAPI("privacy-reports", {leaveOpen: true});

Please change this to "general-search" so that the two different calls to open the preferences are actually opening different subcategories.

Comment 9

[Fischer [:Fischer]]

Comment on attachment 8860408 [details]
Bug 1358475 - The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132420/diff/3-4/

Comment 10

[Fischer [:Fischer]]

Comment on attachment 8860408 [details]
Bug 1358475 - The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory.

https://reviewboard.mozilla.org/r/132420/#review135764

> This commit message explains the bug that is being fixed, but doesn't explain why the new code is better than the old code. Please re-write your commit message.
> 
> Maybe use:
> "Bug 1358475 - The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory. r=jaws
> 
> Computing the subcategory based off of just the hash doesn't work when switchToTabHavingURI is used to open the preferences."

Thanks, definitely will update.

Comment 11

[Fischer [:Fischer]]

Comment on attachment 8860408 [details]
Bug 1358475 - The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory.

https://reviewboard.mozilla.org/r/132420/#review135764

> Please change this to "general-search" so that the two different calls to open the preferences are actually opening different subcategories.

Thanks will do.

Comment 12

[Fischer [:Fischer]]

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f71216fc3496df1ddb226f197ac8f8249349c458

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7faffc4a2ef
The category must be retrieved from either the argument, the hash, or the default category name before computing the subcategory. r=jaws

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Sayed [:LiquidO2]]

I have reproduced this bug using the STR from comment #0 with nightly 55.0a1 (2017-04-21) (64-bit) on Manjaro 17 (64bit).

I can verify that this bug is fixed in Latest Nightly 55.0a1

Build ID 	20170426100344
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170426]