Closed Bug 1250085 Opened 4 years ago Closed 4 years ago

Navigation within an already opened tab doesn't cause a new tabs record to be uploaded

Categories

(Firefox :: Sync, defect, P1)

47 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox47 --- verified
firefox48 --- verified

People

(Reporter: Ovidiu, Assigned: markh)

Details

Attachments

(1 file, 1 obsolete file)

Tested on Mac OS 10.10, Windows 7 x64 
Steps to reproduce:

1 Log in to sync from 2 devices(I used Nightly with 2 profiles on the same OS) with the same account
2 On the first device open 1 tab and go to cnn.com > Go to Sync > Synced is performed 
3 On the second device open Synced Tabs > You can see the tab opened above(cnn.com) 
4 Go again on the first device and in the SAME TAB opened at step 2 > open another site(ex:bbc.com) > Go to Sync > Synced is performed 
5 On the second device in the Synced Tabs Sidebar > click Refresh List
actual results: the site bbc.com is not synced, in the Synced Tabs you can see cnn.com
Expected results: You should see the second site that you opened in the tab
 Note: I have tried 14 times, 2 times it worked, 12 times didn't worked.
I can reproduce on Nightly. Something weird does seem to be happening where we're not detecting future navigations on existing tabs as a signal to change the server state. If you open a new tab, then it's enough.
Flags: firefox-backlog+
Priority: -- → P1
Further investigation is required here.
This kinda sucks as the tab you just walked away from will often not have synced the correct URL.

We currently watch for tabs coming and going, and for tabs being shown, but not for tabs navigating. Fortunately the tabbrowser makes it simple to do this. If this looks good I'll see how painful tests will be...
Assignee: nobody → markh
Status: NEW → ASSIGNED
Attachment #8725156 - Flags: feedback?(rnewman)
Comment on attachment 8725156 [details] [diff] [review]
0001-Bug-1250085-watch-for-tab-navigations-so-we-know-to-.patch

Review of attachment 8725156 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/engines/tabs.js
@@ +399,5 @@
> +    // Whenever we see a "stop" on a document, we note that something has
> +    // changed and we should consider syncing...
> +    if ((flag & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT) &&
> +        (flag & Ci.nsIWebProgressListener.STATE_STOP)) {
> +      this.modified = true;

oh bugger - I accidentally removed a log.debug line - I'll add one back in.
Comment on attachment 8725156 [details] [diff] [review]
0001-Bug-1250085-watch-for-tab-navigations-so-we-know-to-.patch

Review of attachment 8725156 [details] [diff] [review]:
-----------------------------------------------------------------

I'm concerned about perf implications. Is it crazy to unregister the observers on the first change, and re-register them after a tabs upload? Perhaps that's cheaper than running these lines on every page load.

I suggest this because probably no user in the history of the universe has ever had a SCORE_INCREMENT_SMALL bump cause them to sync -- 300 such document loads are required to cause a sync for multi-device users. But those users have a five-minute timer plus sync-on-wake, so that's one page load per second to beat the timer. 

The real value here is in setting `modified` so we know to upload a record at all when only in-tab navigation has occurred, and we can get that by spotting _one_ change.

::: services/sync/modules/engines/tabs.js
@@ +394,5 @@
>      }
>    },
> +
> +  // web progress listeners.
> +  onStateChange(progress, request, flag, status) {

Gosh, I hope this doesn't affect page load times. Talos won't tell us: they don't have Sync enabled. Please measure.

@@ +397,5 @@
> +  // web progress listeners.
> +  onStateChange(progress, request, flag, status) {
> +    // Whenever we see a "stop" on a document, we note that something has
> +    // changed and we should consider syncing...
> +    if ((flag & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT) &&

Perhaps ignore STATE_RESTORING changes?

@@ +399,5 @@
> +    // Whenever we see a "stop" on a document, we note that something has
> +    // changed and we should consider syncing...
> +    if ((flag & Ci.nsIWebProgressListener.STATE_IS_DOCUMENT) &&
> +        (flag & Ci.nsIWebProgressListener.STATE_STOP)) {
> +      this.modified = true;

I don't think we want to log on every single document load!
Attachment #8725156 - Flags: feedback?(rnewman) → feedback+
Summary: When you open 2 sites on the same tab, Sync doesn't work → Navigation within an already opened tab doesn't cause a new tabs record to be uploaded
(In reply to Richard Newman [:rnewman] from comment #5)

> I'm concerned about perf implications. Is it crazy to unregister the
> observers on the first change, and re-register them after a tabs upload?
> Perhaps that's cheaper than running these lines on every page load.

I share your concerns, but I really doubt the gains are worth it here - the code to track this seems far more error prone and fragile

> I suggest this because probably no user in the history of the universe has
> ever had a SCORE_INCREMENT_SMALL bump cause them to sync -- 300 such
> document loads are required to cause a sync for multi-device users. But
> those users have a five-minute timer plus sync-on-wake, so that's one page
> load per second to beat the timer. 
> 
> The real value here is in setting `modified` so we know to upload a record
> at all when only in-tab navigation has occurred, and we can get that by
> spotting _one_ change.

Yeah, I agree the value in incrementing the score is small and that just setting .modified is good enough, at least in the first instance.

> Gosh, I hope this doesn't affect page load times. Talos won't tell us: they
> don't have Sync enabled. Please measure.

I tried using the profiler but the results are so noisy that the cost of this function is lost within it. I also changed the code to use the onLocationChange observer, which seems a better fit for this.  If you look at tabbrowser's existing onLocationChange handler (https://dxr.mozilla.org/mozilla-central/rev/c59c022943f6a7e79f6002e11fc9f56cb836f5dd/browser/base/content/tabbrowser.xml#743) it does enough work that would totally dwarf this change.

> I don't think we want to log on every single document load!

Yeah, fair enough!

I added some tests using the mocks already in test_tab_tracker.
Attachment #8725156 - Attachment is obsolete: true
Attachment #8726065 - Flags: review?(rnewman)
I added a try push that added 5x progress listeners with an identical condition but calling Cu.reportError() instead of just setting a bool (the thinking was that the Cu.reportError() is even more overhead per switch) - https://hg.mozilla.org/try/rev/18472274d192

Strangely, this seems to show a slight (4%) improvement in "a11yr opt e10s" (which doesn't really make sense to me) and no regressions - I'll put that down to cosmic rays or something...

https://treeherder.mozilla.org/perf.html#/compare?originalProject=fx-team&originalRevision=6b7cac27bb83&newProject=try&newRevision=d1778c2c9c8d&framework=1newRevision=d1778c2c9c8d&framework=1
(In reply to Mark Hammond [:markh] from comment #7)
Are you sure you didn't cause an NS_ERROR_FAILURE to get thrown due to https://mxr.mozilla.org/mozilla-central/source/uriloader/base/nsDocLoader.cpp?rev=155ce4441e22&mark=861-863#858 ?
The logs don't show your reportError output but perhaps Talos hides that?
Nevermind comment 8, I confirmed locally that your test worked fine with 5 messages logged each time. I guess Talos hides some output (I do see errors reported earlier in the test).
Attachment #8726065 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/34aa49e08c17
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OS X 10.9.5 using latest Nightly 48.0a1 (buildID: 20160410030224).
I consider it would be a very good idea to uplift this to 47. What do you think, Ritu?
Flags: needinfo?(rkothari)
Hi Mark, Camelia from SoftVision is recommending we uplift this to Aurora 47. It seems like a good idea to me. What do you think? If you agree, could you please nominate a patch for uplift to aurora? Thanks!
Flags: needinfo?(rkothari) → needinfo?(markh)
Comment on attachment 8726065 [details] [diff] [review]
0001-Bug-1250085-watch-for-tab-navigations-so-we-know-to-.patch

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: The list of synced tabs may not show the correct URL for an opened tab.
[Describe test coverage new/current, TreeHerder]: New tests, existing tests passed, been on central for some time with no reported issues or regressions.

[Risks and why]: There's a small risk re performance here as we now attach a global-ish "progress listener" to all open tabs, and this bug has existed forever - it's not a regression, simply an improvement. We also went to lengths to confirm there was no measurable performance impact due to this risk. As recent work has exposed "synced tabs" in a far more obvious way, more users are likely to be exposed to this feature, and thus more likely to notice that some of the tabs are not on the correct URL. For these reasons there are good arguments on both sides of the "uplift or not" question - I'm coming down on the side of "it's unlikely to hurt anything and does make the feature more correct, so let's uplift". YMMV.

[String/UUID change made/needed]: None
Flags: needinfo?(markh)
Attachment #8726065 - Flags: approval-mozilla-aurora?
Oh - I should also have mentioned that the risk is limited to users with Sync configured.
Comment on attachment 8726065 [details] [diff] [review]
0001-Bug-1250085-watch-for-tab-navigations-so-we-know-to-.patch

Fix was verified on Nightly, potential perf impact but could not be quantified/measured, still seems like a good idea to uplift to Aurora47
Attachment #8726065 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OS X 10.9.5 using latest Aurora 47.0a2 (buildID: 20160418004027).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.