1493427 - EV indicator of the previous page persists on error page of an inexistent about: page (about:cofnig / about:lol)

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Darkspirit]

Debian Testing, KDE, Xorg

1. Open https://www.mozilla.org/de/firefox/nightly/firstrun/
2. Now visit about:lol (or about:cofnig) in the same tab
3. There is still an EV indicator: "Mozilla Corporation (US)"

Same regression range as in bug 1490982 comment 4:
mozregression --repo autoland --launch 7056aff16fb8124f1d6043538b9947017c4623e4 -a https://www.mozilla.org/de/firefox/nightly/firstrun/
last good

mozregression --repo autoland --launch 2f4adf14e6231a1668558dd78ecbe56a421591b6 -a https://www.mozilla.org/de/firefox/nightly/firstrun/
first bad

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for filing this. It looks like the failedChannel in docShell is null when nsSecureBrowserUIImpl gets called for this location change event (and the failed channel is what the docShell passes nsSecureBrowserUIImpl). Given that, we don't (can't) notify anything of the security change because that's how we would get our nsISecurityEventSink. From what I can tell, the original implementation would re-use the nsISecurityEventSink from the previous OnLocationChange notification, which seems like a bug. That said, I did a quick test with querying the original docShell to an nsISecurityEventSink and calling OnSecurityChange on that, and it seems to work. Maybe we can just use the original docShell in all cases?

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Attachment: bug 1493427 - use the docShell to call OnSecurityChange in nsSecureBrowserUIImpl::OnLocationChange r?Gijs

When navigating to an about: page that doesn't exist (e.g.
"about:somethingthatdoesnotexist"), the docShell will call
nsSecureBrowserUIImpl::OnLocationChange with a request that is null.
Consequently, we can't use that to QueryInterface to a nsISecurityEventSink to
call OnSecurityChange. The previous implementation would use the prior
request's nsISecurityEventSink, which was a bug but luckily this produced the
correct behavior. Since the original docShell the nsSecureBrowserUIImpl was
initialized with is what needs to be notified, we can just QueryInterface that
to an nsISecurityEventSink and call OnSecurityChange directly instead.

Comment 3

[:Gijs (he/him)]

Comment on attachment 9012276 [details]
bug 1493427 - use the docShell to call OnSecurityChange in nsSecureBrowserUIImpl::OnLocationChange r?Gijs

:Gijs (out Thu 27 - Sun 30 / 9;  he/him) has approved the revision.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the review! https://treeherder.mozilla.org/#/jobs?repo=try&revision=19f8f5cc8e721743d098bacc573f9a4957abd069

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39762ef5d56e
use the docShell to call OnSecurityChange in nsSecureBrowserUIImpl::OnLocationChange r=Gijs

Comment 6

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/39762ef5d56e

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7fa93a38b8bbf23952ee4598b40eedde8b51ffa9

Log:
https://treeherder.mozilla.org/logviewer.html#?job_id=202119350&repo=autoland

 INFO -  TEST-INFO | started process 7240 (C:\Users\task_1538102091\build\application\firefox\firefox -wait-for-browser -no-deelevate -profile c:\users\task_1538102091\appdata\local\temp\tmpe6wabr\profile http://localhost:50064/startup_test/tresize/addon/content/tresize-test.html)
04:19:15     INFO -  TEST-INFO | 7240: exit 1
04:19:22     INFO -  rmtree() failed for "('c:\\users\\task_1538102091\\appdata\\local\\temp\\tmpe6wabr',)". Reason: The process cannot access the file because it is being used by another process (13). Retrying...
04:19:22     INFO -  rmtree() failed for "('c:\\users\\task_1538102091\\appdata\\local\\temp\\tmpe6wabr',)". Reason: The process cannot access the file because it is being used by another process (13). Retrying...
04:19:22     INFO -  rmtree() failed for "('c:\\users\\task_1538102091\\appdata\\local\\temp\\tmpe6wabr',)". Reason: The process cannot access the file because it is being used by another process (13). Retrying...
04:19:22     INFO -  rmtree() failed for "('c:\\users\\task_1538102091\\appdata\\local\\temp\\tmpe6wabr',)". Reason: The process cannot access the file because it is being used by another process (13). Retrying...
04:19:22     INFO -  rmtree() failed for "('c:\\users\\task_1538102091\\appdata\\local\\temp\\tmpe6wabr',)". Reason: The process cannot access the file because it is being used by another process (13). Retrying...
04:19:22     INFO -  Exception while removing profile directory: c:\users\task_1538102091\appdata\local\temp\tmpe6wabr
04:19:22     INFO -  [Error 32] The process cannot access the file because it is being used by another process: 'c:\\users\\task_1538102091\\appdata\\local\\temp\\tmpe6wabr\\profile\\cert9.db'
04:19:22     INFO -  TEST-UNEXPECTED-ERROR | tresize | Could not find report in browser output: [('tsformat', ('__start_report', '__end_report')), ('tpformat', ('__start_tp_report', '__end_tp_report'))]
04:19:22    ERROR -  Traceback (most recent call last):
04:19:22     INFO -    File "C:\Users\task_1538102091\build\tests\talos\talos\run_tests.py", line 303, in run_tests
04:19:22     INFO -      talos_results.add(mytest.runTest(browser_config, test))
04:19:22     INFO -    File "C:\Users\task_1538102091\build\tests\talos\talos\ttest.py", line 63, in runTest
04:19:22     INFO -      return self._runTest(browser_config, test_config, setup)
04:19:22     INFO -    File "C:\Users\task_1538102091\build\tests\talos\talos\ttest.py", line 277, in _runTest
04:19:22     INFO -      else None)
04:19:22     INFO -    File "C:\Users\task_1538102091\build\tests\talos\talos\results.py", line 95, in add
04:19:22     INFO -      global_counters=self.global_counters
04:19:22     INFO -    File "C:\Users\task_1538102091\build\tests\talos\talos\results.py", line 326, in __init__
04:19:22     INFO -      self.parse()
04:19:22     INFO -    File "C:\Users\task_1538102091\build\tests\talos\talos\results.py", line 353, in parse
04:19:22     INFO -      % self.report_tokens)
04:19:22     INFO -    File "C:\Users\task_1538102091\build\tests\talos\talos\results.py", line 337, in error
04:19:22     INFO -      raise utils.TalosError(message)
04:19:22     INFO -  TalosError: Could not find report in browser output: [('tsformat', ('__start_report', '__end_report')), ('tpformat', ('__start_tp_report', '__end_tp_report'))]
04:19:22     INFO -  TEST-INFO took 105690ms
04:19:22     INFO -  SUITE-END | took 218s
04:22:20     INFO -  WARNING | IO Completion Port failed to signal process shutdown
04:22:20     INFO -  Parent process 1456 exited with children alive:
04:22:20     INFO -  PIDS: 8400, 2664, 9040, 4624, 2776, 6364
04:22:20     INFO -  Attempting to kill them, but no guarantee of success
04:22:20    ERROR - Return code: 2
04:22:20  WARNING - setting return code to 2
04:22:20    ERROR - # TBPL FAILURE #
04:22:20     INFO - Running post-action listener: _package_coverage_data
04:22:20     INFO - Running post-action listener: _resource_record_post_action
04:22:20     INFO - Running post-action listener: process_java_coverage_data
04:22:20     INFO - [mozharness: 2018-09-28 04:22:20.496000Z] Finished run-tests step (success)
04:22:20     INFO - Running post-run listener: _resource_record_post_run
04:22:20     INFO - Total resource usage - Wall time: 398s; CPU: 5.0%; Read bytes: 45225472; Write bytes: 267981824; Read time: 1; Write time: 4
04:22:20     INFO - TinderboxPrint: CPU usage<br/>4.9%
04:22:20     INFO - TinderboxPrint: I/O read bytes / time<br/>45,225,472 / 1
04:22:20     INFO - TinderboxPrint: I/O write bytes / time<br/>267,981,824 / 4
04:22:20     INFO - TinderboxPrint: CPU idle<br/>3,025.5 (95.0%)
04:22:20     INFO - TinderboxPrint: CPU system<br/>39.2 (1.2%)
04:22:20     INFO - TinderboxPrint: CPU user<br/>117.8 (3.7%)
04:22:20     INFO - install - Wall time: 2s; CPU: 13.0%; Read bytes: 4096; Write bytes: 9643520; Read time: 0; Write time: 0
04:22:20     INFO - setup-mitmproxy - Wall time: 0s; CPU: Can't collect data; Read bytes: 0; Write bytes: 0; Read time: 0; Write time: 0
04:22:20     INFO - run-tests - Wall time: 397s; CPU: 5.0%; Read bytes: 41814528; Write bytes: 256793088; Read time: 1; Write time: 4
04:22:20     INFO - Running post-run listener: copy_logs_to_upload_dir
04:22:20     INFO - Copying logs to upload dir...
04:22:20     INFO - mkdir: C:\Users\task_1538102091\build\upload\logs
04:22:20     INFO - Copying logs to upload dir...
04:22:20     INFO - Using _rmtree_windows ...
04:22:20     INFO - Running command: del /F /Q "C:\Users\task_1538102091\build\upload\logs\localconfig.json"
04:22:20     INFO - Return code: 0

The increase in frequency was introduced with push https://hg.mozilla.org/integration/autoland/rev/39762ef5d56e

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler]]

With bug 1494573 fixed and rebasing off of bug 1493563, try now looks like this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e68381917108786d14b318103c5bc93ea05297d

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84b4b9b7586a
use the docShell to call OnSecurityChange in nsSecureBrowserUIImpl::OnLocationChange r=Gijs

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/84b4b9b7586a