Closed Bug 1243778 Opened 4 years ago Closed 4 years ago

PushRecord::getLastVisit cannot rely on the Places url index anymore

Categories

(Core :: DOM: Push Notifications, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

The problem is this query

150         `SELECT MAX(p.last_visit_date)
151          FROM moz_places p
152          INNER JOIN moz_historyvisits h ON p.id = h.place_id
153          WHERE (
154            p.url >= :urlLowerBound AND p.url <= :urlUpperBound AND
155            h.visit_type IN (${QUOTA_REFRESH_TRANSITIONS_SQL})
156          )

it relies on the url index, but we will remove that index in bug 889561, so the query should be rewritten to workaround it.
It should be feasible by using the rev_host column to restrict the dataset first.
Priority: -- → P2
Whiteboard: [fxsearch]
Something like this should do:

SELECT MAX(visit_date)
          FROM moz_places p
          JOIN moz_historyvisits ON p.id = place_id
          WHERE
          rev_host = get_unreversed_host(:host || '.') || '.'
          AND url BETWEEN :prePath AND :prePath || X'FFFF'
          AND visit_type IN (1,2,3,5,6)

note the original query had a bug, since it was using last_visit_date that is an aggregate that also includes the visit types that the query wants to exclude.
I'm trying to run tests in dom/push, but dom/push/test/xpcshell/test_quota_exceeded.js constantly fails, even without my patch. I'm on Windows 10, and this is the error:

0:16.55 PROCESS_OUTPUT: Thread-3 (pid:532) "console.debug: PushDB: "
 0:16.55 PROCESS_OUTPUT: Thread-3 (pid:532) "  update: Update successful"
 0:16.55 PROCESS_OUTPUT: Thread-3 (pid:532) "  46cc6f6a-c106-4ffa-bb7c-55c60bd50c41"
 0:16.55 PROCESS_OUTPUT: Thread-3 (pid:532) "Object"
 0:16.55 PROCESS_OUTPUT: Thread-3 (pid:532) "    - pushEndpoint = https://example.org/push/2"
 0:16.55 PROCESS_OUTPUT: Thread-3 (pid:532) "    - scope = https://example.com/deals"
 0:16.55 PROCESS_OUTPUT: Thread-3 (pid:532) "    - originAttributes = "
 0:16.55 PROCESS_OUTPUT: Thread-3 (pid:532) "    - pushCount = 1"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "    - lastPush = 1454951261732"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "    - p256dhPublicKey = undefined"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "    - p256dhPrivateKey = undefined"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "    - authenticationSecret = undefined"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "    - systemRecord = false"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "    - quota = 5"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "    - ctime = 0"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "    - channelID = 46cc6f6a-c106-4ffa-bb7c-55c60bd50c41"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "    - version = 1"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "console.debug: PushService: "
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "  notifyApp()"
 0:16.56 PROCESS_OUTPUT: Thread-3 (pid:532) "  https://example.com/deals"
 0:19.23 LOG: Thread-3 ERROR Unexpected exception Error: Timed out waiting for unregister request at e:/mozilla/src/obj-i686-pc-mingw32/_tests/xpcshell/dom/push/test/xpcshell/head.js:127

any idea?

Btw, I will proceed with the patch and a Try run, supposing the above failure in unrelated.
Flags: needinfo?(kcambridge)
Hmm, does it help if you bump the timeout? https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/push/test/xpcshell/test_quota_exceeded.js#138-139 The default is 5 seconds, which makes me suspect it's something else.

Please ignore the failure for now; I can take a look later this week.
Flags: needinfo?(kcambridge)
(In reply to Kit Cambridge [:kitcambridge] from comment #3)
> Hmm, does it help if you bump the timeout?

nope, no luck.
I wonder if it's a timezone thing... I suppose you are doing some time calculations there. I'm on CET.
Comment on attachment 8717016 [details]
MozReview Request: Bug 1243778 - PushRecord::getLastVisit cannot rely on the Places url index anymore. r=kitcambridge

https://reviewboard.mozilla.org/r/34011/#review30647

Thanks, :mak!

::: dom/push/PushRecord.jsm:22
(Diff revision 1)
> -
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",

Nice. :-)

::: toolkit/components/places/tests/PlacesTestUtils.jsm:39
(Diff revision 1)
> -  addVisits: Task.async(function*(placeInfo) {
> +  addVisits: Task.async(function* (placeInfo) {

\o/
Attachment #8717016 - Flags: review?(kcambridge) → review+
(In reply to Marco Bonardo [::mak] from comment #7)
> I wonder if it's a timezone thing... I suppose you are doing some time
> calculations there. I'm on CET.

I was curious about that, too. Tried setting my clock to CET (and NZDT, for good measure), but couldn't get it to fail on OS X or Ubuntu. I'll try it on my Windows box tomorrow. Also, could you paste the full output, please? I wonder if it's even calling `backgroundUnregister`...
well, after all it may not just be me (that's strange, unless I had to build more than just dom/), the Try run shows errors exactly in that test :/

You can check the log here
https://treeherder.mozilla.org/logviewer.html#?job_id=16491612&repo=try
or install the patch locally and see if you notice anything

I'll see if I can figure out something, but I don't know much about how this stuff is supposed to work, so any help is welcome.
comparing the logs, the only thing I can tell for now it's the pass test at a given point has push1 changing from quota: 16 to quota: 5, the failing test retains quota 16
sounds like updateQuota() is not working properly with my patch. Indeed Math.round(8 * Math.pow(2, -0.8)) = 5 and there is a 2 days old visit in the test, that is what we should return from getLastVisit.
At least I have a trace to look at.
ah sigh, it was a very dumb thing, I didn't rename a transitionType to transition in test_quota_exceeded.js and thus the test was correctly failing.
Also, looks like rebuilding just dom/ is not enough to update the module code, but build faster seems to do the trick.

The change is trivial (transitionType to transition), sorry for any time you may have lost figuring it out.
Comment on attachment 8717016 [details]
MozReview Request: Bug 1243778 - PushRecord::getLastVisit cannot rely on the Places url index anymore. r=kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34011/diff/1-2/
(In reply to Marco Bonardo [::mak] from comment #13)
> ah sigh, it was a very dumb thing, I didn't rename a transitionType to
> transition in test_quota_exceeded.js and thus the test was correctly failing.

Oops. Those one-line bugs are the worst. Thank you for catching that, and for the patch!
https://hg.mozilla.org/mozilla-central/rev/802e3c169db8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.