1581403 - onLocationChange is sometimes not triggered for about:blank loads

Status: NEW

(Firefox :: Tabbed Browser, defect, P3)

Description

[Rob Wu [:robwu]]

Some code relies on the onLocationChange event in order to detect that a load after a gBrowser.addTab call has finished.

Apparently, in the case of about:blank, there is a (small) possibility that the onLocationChange is never fired. This was observed while debugging bug 1527029. I have added lots of details below, and summarized the findings here. Event sequences after a gBrowser.addTab call are consistent within one group. The good cases are noticeably different from the bad cases.

In the good cases, the event sequence upon calling gBrowser.addTab is:

1. Create TabProgressListener (very rarely, step 2 runs first - creation of inner and outer window)
   (now, gBrowser.addTab returns)
2. (content process) Create outer window and inner window.
3. Trigger onStateChange with flags including STATE_IS_STOP (c0010).
4. (content process) Create second inner window (may also happen after the next step).
5. Trigger onStateChange with flags including STATE_IS_START (f0001).
6. Trigger onLocationChange.
7. Trigger onStateChange with flags including STATE_IS_STOP (c0010).

In the bad cases, the event sequence is:

1. (content process) Create outer window and inner window.
2. (content process) Create second inner window.
   (note: step 1 and 2 executing before 3 means that the parent process has yielded to the content process)
3. Create TabProgressListener.
4. (content process) Create third inner window. (?????????????????)
   (now, gBrowser.addTab returns)
5. Trigger onStateChange with flags including STATE_IS_START (f0001).
6. Update location information, but do not forward onLocationChange (because there was no onStateChange with STATE_IS_STOP before this point).
7. Trigger onStateChange with flags including STATE_IS_STOP (c0010).

At step 6, onLocationChange should have been triggered (and not doing so causes other bugs, such as bug 1527029). The bug seems to happen because the onStateChange notification for the initial about:blank with flags including STATE_IS_STOP was not received.

---

---

Detailed version

• Isolated test case: https://hg.mozilla.org/try/rev/c526309a45f4017b75459730e4206a0f4d0b5761 ( https://phabricator.services.mozilla.com/D45955 )
• More verbose logging: https://hg.mozilla.org/try/rev/d71eab7572bbcdd07791e93637136dfb52f3b2c0
• Try results, TV (--verify): https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=266707309&revision=84a7d91f7b33418bcf9d980026485c8cf4211d9e&searchStr=tv

The points of interest are the log points at:

• Constructor of TabProgressListener - shows when the TabProgressListener is being constructed (via _insertBrowser , via tabbrowser.addTab).
• The ++DOMWINDOW entries that are generated in debug builds.
• tabbrowser's TabProgressListener.onLocationChange, which is responsible for dispatching onLocationChange to progress listeners, but only if mBlank is false.
• tabbrowser's TabProgressListener.onStateChange, which is responsible for setting mBlank to true if needed.
• ( updateForLocationChange in browser-custom-element.js, because it is always called before onLocationChange is called. This method is responsible for updating information such as the tab browser's innerWindowID in which I am interested. )
• ( after creating the tab, the test waits until resolution of awaitTabReady, which depends on onLocationChange. When this bug is triggered, the test will time out (bug 1527029) )

In the onStateChange logs below, aStateFlags has two values:

• aStateFlags=c0010 above is (STATE_IS_*) NETWORK | DOCUMENT | STOP
• aStateFlags=f0001 above is (STATE_IS_*) WINDOW | NETWORK | DOCUMENT | REQUEST | START

The significance of these is that mBlank is only flipped to true in the c0010 case.

awaitTabReady=true and awaitTabReady=false also appear. The exact value does not matter; true means that onLocationChange was not triggered at the time that the test awaits the result, false means that onLocationChange was already observed. Either case is fine, but if awaitTabReady=true, then the test will wait until onLocationChange is triggered.

In the common, expected good case, the logs are as follows (all from https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=266707309&revision=84a7d91f7b33418bcf9d980026485c8cf4211d9e&searchStr=tv ):

TabProgressListener
awaitTabReady=true innerWindowID=null
++DOMWINDOW == 1 (0x12863c020) [pid = 2081] [serial = 1] [outer = 0x0]
++DOMWINDOW == 2 (0x122ad3400) [pid = 2081] [serial = 2] [outer = 0x12863c020]
onStateChange innerWindowID=null mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
++DOMWINDOW == 3 (0x122ad7c00) [pid = 2081] [serial = 3] [outer = 0x12863c020]
onStateChange innerWindowID=null mBlank=false wasBlank=false ignoreBlank=false aStateFlags=f0001
updateForLocationChange innerWindowID=8589934593
onLocationChange innerWindowID=8589934593 mBlank=false hasContentOpener=false
onStateChange innerWindowID=8589934593 mBlank=false wasBlank=false ignoreBlank=false aStateFlags=c0010

TabProgressListener
awaitTabReady=true innerWindowID=null
++DOMWINDOW == 1 (0x11cb41020) [pid = 2084] [serial = 1] [outer = 0x0]
++DOMWINDOW == 2 (0x116fd9400) [pid = 2084] [serial = 2] [outer = 0x11cb41020]
onStateChange innerWindowID=null mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
++DOMWINDOW == 3 (0x105026400) [pid = 2084] [serial = 3] [outer = 0x11cb41020]
onStateChange innerWindowID=null mBlank=false wasBlank=false ignoreBlank=false aStateFlags=f0001
updateForLocationChange innerWindowID=15032385537
onLocationChange innerWindowID=15032385537 mBlank=false hasContentOpener=false
onStateChange innerWindowID=15032385537 mBlank=false wasBlank=false ignoreBlank=false aStateFlags=c0010

TabProgressListener
awaitTabReady=true innerWindowID=null
++DOMWINDOW == 1 (0x122d40020) [pid = 2086] [serial = 1] [outer = 0x0]
++DOMWINDOW == 2 (0x11a1d8c00) [pid = 2086] [serial = 2] [outer = 0x122d40020]
onStateChange innerWindowID=null mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
onStateChange innerWindowID=null mBlank=false wasBlank=false ignoreBlank=false aStateFlags=f0001
++DOMWINDOW == 3 (0x122d99800) [pid = 2086] [serial = 3] [outer = 0x122d40020]
updateForLocationChange innerWindowID=19327352833
onLocationChange innerWindowID=19327352833 mBlank=false hasContentOpener=false
onStateChange innerWindowID=19327352833 mBlank=false wasBlank=false ignoreBlank=false aStateFlags=c0010

In this case, the parent process seems to yield to the content process, which allows the latter to create the outer and inner windows before the gBrowser.addTab call returns (as seen by TabProgressListener executing after the ++DOMWINDOW).

++DOMWINDOW == 3 (0x120140200) [pid = 2080] [serial = 3] [outer = 0x0]
++DOMWINDOW == 4 (0x1195dd000) [pid = 2080] [serial = 4] [outer = 0x120140200]
TabProgressListener
++DOMWINDOW == 5 (0x12018b400) [pid = 2080] [serial = 5] [outer = 0x120140200]
awaitTabReady=true innerWindowID=null
onStateChange innerWindowID=null mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
onStateChange innerWindowID=null mBlank=false wasBlank=false ignoreBlank=false aStateFlags=f0001
updateForLocationChange innerWindowID=6442450945
onLocationChange innerWindowID=6442450945 mBlank=false hasContentOpener=false
onStateChange innerWindowID=6442450945 mBlank=false wasBlank=false ignoreBlank=false aStateFlags=c0010

Unlike the previous ones, the next example creates 3 inner windows instead of 2, and sends two innerWindowID updates. Only the second update triggers onLocationChange calls (as seen by mBlank=false). It seems that the mBlank guard is designed to catch this kind of event sequence (and does so to prevent onLocationChange to firing too early).
If the second inner window ([serial = 6]) were to be created before the TabProgressListener's construction, then the event sequence looks very similar to the failing cases below, and it would most likely have failed the test instead of passing it.

++DOMWINDOW == 4 (0x12863c3e0) [pid = 2081] [serial = 4] [outer = 0x0]
++DOMWINDOW == 5 (0x122acc800) [pid = 2081] [serial = 5] [outer = 0x12863c3e0]
TabProgressListener
++DOMWINDOW == 6 (0x128f96400) [pid = 2081] [serial = 6] [outer = 0x12863c3e0]
++DOMWINDOW == 7 (0x128f9a400) [pid = 2081] [serial = 7] [outer = 0x12863c3e0]
updateForLocationChange innerWindowID=8589934594
onLocationChange innerWindowID=8589934594 mBlank=true hasContentOpener=false
onStateChange innerWindowID=8589934594 mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
onStateChange innerWindowID=8589934594 mBlank=false wasBlank=false ignoreBlank=false aStateFlags=f0001
updateForLocationChange innerWindowID=8589934595
onLocationChange innerWindowID=8589934595 mBlank=false hasContentOpener=false
onStateChange innerWindowID=8589934595 mBlank=false wasBlank=false ignoreBlank=false aStateFlags=c0010
awaitTabReady=false innerWindowID=8589934595

In the rare, broken case, the logs are as follows. Note that the onLocationChange line contains mBlank=true, which prevents onLocationChange from being dispatched to the progress listeners, resulting in bugs such as bug 1527029.

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=266707309&revision=84a7d91f7b33418bcf9d980026485c8cf4211d9e&searchStr=tv fails with:

++DOMWINDOW == 10 (0x1201407a0) [pid = 2080] [serial = 10] [outer = 0x0]
++DOMWINDOW == 11 (0x118d91400) [pid = 2080] [serial = 11] [outer = 0x1201407a0]
++DOMWINDOW == 12 (0x11a5d9800) [pid = 2080] [serial = 12] [outer = 0x1201407a0]
TabProgressListener
++DOMWINDOW == 13 (0x120e4e000) [pid = 2080] [serial = 13] [outer = 0x1201407a0]
onStateChange innerWindowID=null mBlank=true wasBlank=true ignoreBlank=true aStateFlags=f0001
updateForLocationChange innerWindowID=6442450949
onLocationChange innerWindowID=6442450949 mBlank=true hasContentOpener=false
onStateChange innerWindowID=6442450949 mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
awaitTabReady=true innerWindowID=6442450949

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=266765887&revision=84a7d91f7b33418bcf9d980026485c8cf4211d9e&searchStr=tv fails with:

++DOMWINDOW == 4 (0x1228443e0) [pid = 2102] [serial = 4] [outer = 0x0]
++DOMWINDOW == 5 (0x11ccd1800) [pid = 2102] [serial = 5] [outer = 0x1228443e0]
++DOMWINDOW == 6 (0x123093400) [pid = 2102] [serial = 6] [outer = 0x1228443e0]
TabProgressListener
++DOMWINDOW == 7 (0x123097800) [pid = 2102] [serial = 7] [outer = 0x1228443e0]
onStateChange innerWindowID=null mBlank=true wasBlank=true ignoreBlank=true aStateFlags=f0001
updateForLocationChange innerWindowID=10737418243
onLocationChange innerWindowID=10737418243 mBlank=true hasContentOpener=false
onStateChange innerWindowID=10737418243 mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
awaitTabReady=true innerWindowID=10737418243

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=266765889&revision=84a7d91f7b33418bcf9d980026485c8cf4211d9e&searchStr=tv fails with:

++DOMWINDOW == 4 (0x12703f3e0) [pid = 2168] [serial = 4] [outer = 0x0]
++DOMWINDOW == 5 (0x10ed29000) [pid = 2168] [serial = 5] [outer = 0x12703f3e0]
++DOMWINDOW == 6 (0x12789d800) [pid = 2168] [serial = 6] [outer = 0x12703f3e0]
TabProgressListener
++DOMWINDOW == 7 (0x1278a1800) [pid = 2168] [serial = 7] [outer = 0x12703f3e0]
onStateChange innerWindowID=null mBlank=true wasBlank=true ignoreBlank=true aStateFlags=f0001
updateForLocationChange innerWindowID=8589934595
onLocationChange innerWindowID=8589934595 mBlank=true hasContentOpener=false
onStateChange innerWindowID=8589934595 mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
awaitTabReady=true innerWindowID=8589934595 

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=266765890&revision=84a7d91f7b33418bcf9d980026485c8cf4211d9e&searchStr=tv fails with:

++DOMWINDOW == 3 (0x122541200) [pid = 2101] [serial = 3] [outer = 0x0]
++DOMWINDOW == 4 (0x11b9d9c00) [pid = 2101] [serial = 4] [outer = 0x122541200]
++DOMWINDOW == 5 (0x1225cec00) [pid = 2101] [serial = 5] [outer = 0x122541200]
TabProgressListener
++DOMWINDOW == 6 (0x1228a8000) [pid = 2101] [serial = 6] [outer = 0x122541200]
onStateChange innerWindowID=null mBlank=true wasBlank=true ignoreBlank=true aStateFlags=f0001
updateForLocationChange innerWindowID=6442450946
onLocationChange innerWindowID=6442450946 mBlank=true hasContentOpener=false
onStateChange innerWindowID=6442450946 mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
awaitTabReady=true innerWindowID=6442450946

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=266765892&revision=84a7d91f7b33418bcf9d980026485c8cf4211d9e&searchStr=tv fails with:

++DOMWINDOW == 4 (0x11a03f3e0) [pid = 2113] [serial = 4] [outer = 0x0]
++DOMWINDOW == 5 (0x101d2a000) [pid = 2113] [serial = 5] [outer = 0x11a03f3e0]
++DOMWINDOW == 6 (0x11ab95c00) [pid = 2113] [serial = 6] [outer = 0x11a03f3e0]
TabProgressListener
++DOMWINDOW == 7 (0x11ab9a000) [pid = 2113] [serial = 7] [outer = 0x11a03f3e0]
onStateChange innerWindowID=null mBlank=true wasBlank=true ignoreBlank=true aStateFlags=f0001
updateForLocationChange innerWindowID=8589934595
onLocationChange innerWindowID=8589934595 mBlank=true hasContentOpener=false
onStateChange innerWindowID=8589934595 mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
awaitTabReady=true innerWindowID=8589934595

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=266765905&revision=84a7d91f7b33418bcf9d980026485c8cf4211d9e&searchStr=tv fails with:

++DOMWINDOW == 15 (0x118d42b60) [pid = 2200] [serial = 21] [outer = 0x0]
++DOMWINDOW == 16 (0x11959d000) [pid = 2200] [serial = 22] [outer = 0x118d42b60]
++DOMWINDOW == 17 (0x13a5b8400) [pid = 2200] [serial = 23] [outer = 0x118d42b60]
TabProgressListener
++DOMWINDOW == 18 (0x13ce74400) [pid = 2200] [serial = 24] [outer = 0x118d42b60]
onStateChange innerWindowID=null mBlank=true wasBlank=true ignoreBlank=true aStateFlags=f0001
updateForLocationChange innerWindowID=6442450954
onLocationChange innerWindowID=6442450954 mBlank=true hasContentOpener=false
onStateChange innerWindowID=6442450954 mBlank=false wasBlank=true ignoreBlank=true aStateFlags=c0010
awaitTabReady=true innerWindowID=6442450954