1168606 - Implement preloading cursors for IndexedDB

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, task, P2)

Description

[Biru [:poiru]]

Cursors should be able to preload records in the background as needed rather than loading everything at once.

Comment 1

[Biru [:poiru]]

Attachment: Part 1: Extract Cursor response data population into shared function

Comment 2

[Biru [:poiru]]

Attachment: Part 2: Convert DelayedDeleteRunnable into reusable DelayedActionRunnable class

This be used for a delayed continue runnable later on.

Comment 3

[Biru [:poiru]]

Attachment: Part 3: Change ObjectStoreCursorResponse to contain an array of records

For now, we assume that it always carries a single record.

Comment 4

[Biru [:poiru]]

Attachment: Part 4: Add CursorRequest type that wraps CursorRequestParams

This is in preparation for a future patch that will include the current key in
the request.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8617354 [details] [diff] [review]
Part 1: Extract Cursor response data population into shared function

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

::: dom/indexedDB/ActorsParent.cpp
@@ +24518,5 @@
>  }
>  
> +nsresult
> +Cursor::
> +CursorOpBase::PopulateResponseFromStatement(

I'd assert here that you're being called on the right thread, this should do it:

  Transaction()->AssertIsOnConnectionThread();

I'd also like to see a few other asserts:

  MOZ_ASSERT(mResponse.type() == CursorResponse::T__None);
  MOZ_ASSERT(mFiles.IsEmpty());

@@ +24525,5 @@
> +  nsresult rv;
> +
> +  switch (mCursor->mType) {
> +    case OpenCursorParams::TObjectStoreOpenCursorParams: {
> +      rv = mCursor->mKey.SetFromStatement(aStmt, 0);

Currently this is called with the same args in all four cases, so move before the switch statement?

@@ +24732,5 @@
>      mResponse = void_t();
>      return NS_OK;
>    }
>  
> +  rv = PopulateResponseFromStatement(stmt);

So now that you're splitting the part where we build the query from the part where we read the results I think we should have some big comments in all the places where we build the query that changing the number or order of SELECT columns will require changes to the PopulateResponseFromStatement function. Otherwise it might be easy to make changes in one spot without remembering the other.

@@ +25553,2 @@
>    bool hasResult;
>    for (uint32_t index = 0; index < advanceCount; index++) {

One bug I happened to notice here, would you mind adding an assertion right after we set |advanceCount| that |advanceCount| is never 0?

And could you add a check for this in |TransactionBase::VerifyRequestParams(const CursorRequestParams& aParams)| also? Something like:

  if (NS_WARN_IF(!aParams.get_AdvanceParams().count())) {
    ASSERT_UNLESS_FUZZING();
    return false;
  }

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8617357 [details] [diff] [review]
Part 2: Convert DelayedDeleteRunnable into reusable DelayedActionRunnable class

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

This looks fine but I'd much rather get rid of this DelayedDeleteRunnable custom class altogether and use NS_NewRunnableMethod[WithArg]. How does that sound?

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8621233 [details] [diff] [review]
Part 3: Change ObjectStoreCursorResponse to contain an array of records

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

::: dom/indexedDB/PBackgroundIDBCursor.ipdl
@@ +43,5 @@
>  };
>  
> +struct ObjectStoreCursorResponse
> +{
> +  ObjectStoreCursorRecord[] records;

Hm, I don't think this is the right direction really (we want to do preloading cursors for all the different cursor types). Instead how about you just change CursorResponse:

  union CursorResponse
  {
    void_t;
    nsresult;
    ObjectStoreCursorResponse[];
    ObjectStoreKeyCursorResponse[];
    IndexCursorResponse[];
    IndexKeyCursorResponse[];
  };

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8621235 [details] [diff] [review]
Part 4: Add CursorRequest type that wraps CursorRequestParams

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

This way would work, but I think we can do it simpler:

::: dom/indexedDB/PBackgroundIDBCursor.ipdl
@@ +37,5 @@
>  };
>  
> +struct CursorRequest
> +{
> +  CursorRequestParams params;

Hm, instead of doing this, how about you just add the key as a separate arg to Continue()? E.g.:

  Continue(CursorRequestParams params, Key key);

Comment 9

[Biru [:poiru]]

Attachment: Part 3: Allow multiple ObjectStoreCursorResponses in a CursorResponse

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #7)
> Comment on attachment 8621233 [details] [diff] [review]
> Part 3: Change ObjectStoreCursorResponse to contain an array of records
> 
> Review of attachment 8621233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/indexedDB/PBackgroundIDBCursor.ipdl
> @@ +43,5 @@
> >  };
> >  
> > +struct ObjectStoreCursorResponse
> > +{
> > +  ObjectStoreCursorRecord[] records;
> 
> Hm, I don't think this is the right direction really (we want to do
> preloading cursors for all the different cursor types). Instead how about
> you just change CursorResponse:
> 
>   union CursorResponse
>   {
>     void_t;
>     nsresult;
>     ObjectStoreCursorResponse[];
>     ObjectStoreKeyCursorResponse[];
>     IndexCursorResponse[];
>     IndexKeyCursorResponse[];
>   };

Done. (For some reason I assumed arrays couldn't be stored in IPDL unions...)

Comment 10

[Biru [:poiru]]

Attachment: Part 4: Allow current key to be provided with PBackgroundIDBCursor::Continue

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #8)
> Hm, instead of doing this, how about you just add the key as a separate arg
> to Continue()? E.g.:
> 
>   Continue(CursorRequestParams params, Key key);

Done.

Comment 11

[Biru [:poiru]]

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #6)
> Comment on attachment 8617357 [details] [diff] [review]
> Part 2: Convert DelayedDeleteRunnable into reusable DelayedActionRunnable
> class
> 
> Review of attachment 8617357 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine but I'd much rather get rid of this DelayedDeleteRunnable
> custom class altogether and use NS_NewRunnableMethod[WithArg]. How does that
> sound?

DelayedDeleteRunnable implements nsICancellableRunnable while NS_NewRunnableMethodWithArg does not. Does that matter?

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Birunthan Mohanathas [:poiru] from comment #11)
> DelayedDeleteRunnable implements nsICancellableRunnable while
> NS_NewRunnableMethodWithArg does not. Does that matter?

Ugh, yes. It does, thanks for catching that.

Comment 13

[Biru [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e99b141dd96b
https://hg.mozilla.org/integration/mozilla-inbound/rev/05a59f43bb02

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e99b141dd96b
https://hg.mozilla.org/mozilla-central/rev/05a59f43bb02

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8627381 [details] [diff] [review]
Part 3: Allow multiple ObjectStoreCursorResponses in a CursorResponse

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

::: dom/indexedDB/ActorsParent.cpp
@@ +7590,5 @@
>    Cleanup() override;
>  
>    nsresult
> +  PopulateResponseFromStatement(DatabaseConnection::CachedStatement& aStmt,
> +                                bool aInitializeResponse);

Boolean parameters suck.  Can you make this a more descriptive enum?  Perhaps

enum InitializeResponseType {
  InitializeResponse,
  DoNotIntializeResponse
};

I'm also assuming a later patch is going to call this with false ...

Comment 16

[Biru [:poiru]]

Attachment: Part 5: Send multiple records at once in continue response

This is a WIP patch that sends 2 records for each continue response. This is not exactly what we want, but I just wanted to ask for feedback about the general approach before I polish this further.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8639389 [details] [diff] [review]
Part 5: Send multiple records at once in continue response

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

This looks reasonable to me.

Comment 18

[Biru [:poiru]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=986446bc5da9

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f31c24e2ca0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e9fb56cc22a
https://hg.mozilla.org/integration/mozilla-inbound/rev/13e00068cb2b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1f31c24e2ca0
https://hg.mozilla.org/mozilla-central/rev/8e9fb56cc22a
https://hg.mozilla.org/mozilla-central/rev/13e00068cb2b

Comment 21

[Andrew Sutherland [:asuth] (he/him)]

Should this bug be marked fixed and/or a follow-up bug spun off?  The initial bunch of commits in comment 14 added leave-open, but then a bunch more landed in comment 20, albeit with comment 16 calling the patch "WIP"?

Comment 22

[Biru [:poiru]]

(In reply to Andrew Sutherland [:asuth] from comment #21)
> Should this bug be marked fixed and/or a follow-up bug spun off?  The
> initial bunch of commits in comment 14 added leave-open, but then a bunch
> more landed in comment 20, albeit with comment 16 calling the patch "WIP"?

With the "WIP" patch, we send two records with each ObjectStoreCursorResponse. This is better than nothing, but there is still a lot of work to be done. I landed those patches after discussing the situation (I was not going to be able to work on them for a while) with khuey. I'm back now and will probably get back to this as time permits.

I'm going to leave this open. If you prefer, I am also happy to spin the remaining work into a follow-up bug.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Please open a new bug for any remaining work.

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

Reopen this bug because patch#4 & patch#5 are dead codes as explain in bug 1289375 comment 0.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

For qf context, see bug 1486980 comment 32 and down.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Also see bug 1499550.

Comment 27

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Fixed wrong method name in log message. r=ttung

Comment 28

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Reduce the use of plain pointers, and use const for Cursor data members where possible. r=ttung

Depends on D40953

Comment 29

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Change definition of OpenCursorParams subtypes to allow reducing duplicated code r=ttung

Depends on D40954

Comment 30

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Reduce code duplication for different open cursor variants. r=ttung

Depends on D40955

Comment 31

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Reduce code duplication for different cursor directions. r=ttung

Depends on D40956

Comment 32

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Removed unused methods in IDBKeyRange. r=ttung

The functionality has been moved to DatabaseOperationBase::BindKeyRangeToStatement
resp. DatabaseOperationBase::GetBindingClauseForKeyRange as part of Bug 994190.

Depends on D40957

Comment 33

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Extracted methods Cursor::OpenOp::PrepareKeyConditionClauses and PrepareIndexKeyConditionClause. r=ttung

Also reduced duplication within the extracted methods.

Depends on D41024

Comment 34

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Unified handling of literal strings using NS_LITERAL_(C)STRING and removed some duplications. r=ttung

Depends on D41025

Comment 35

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Added missing assignment of Exists result to rv. r=ttung

Depends on D41026

Comment 36

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Introduce macros RETURN_NSRESULT_IF_FAILED_AND_WARN and RETURN_VOID_IF_FAILED_AND_WARN for typical control flow with functions returning an nsresult. r=ttung

Depends on D41191

Comment 37

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Extract DatabaseConnection::ExecuteCachedStatement function. r=ttung

Depends on D41192

Comment 38

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Added documentation on Cursor data members. r=ttung

Depends on D41225

Comment 39

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Move the functionality of OpenOp::GetKeyRangeInfo to Cursor::SetOptionalKeyRange r=ttung

Depends on D41403

Comment 40

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Added comments on almost-const data members r=ttung

Depends on D41404

Comment 41

[Simon Giesecke [:sg] [he/him]]

Please land revisions D40953, D40957, D41024 and D41191.

Do not land D41403 yet, it requires changes.

The other revisions have not yet completed review.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4f3b348afe01
Fixed wrong method name in log message. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/26a290615bbd
Reduce code duplication for different cursor directions. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/f0f5d12cc2b7
Removed unused methods in IDBKeyRange. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/fa95640baa86
Added missing assignment of Exists result to rv. r=ttung,asuth

Comment 43

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

In the future, please order the changesets that the ones ready to land are directly after the base commit else the automatic landing system Lando will block them. This can be done with Edit Related Revision on the right side of a phabricator page.

Comment 44

[Simon Giesecke [:sg] [he/him]]

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #43)

In the future, please order the changesets that the ones ready to land are directly after the base commit else the automatic landing system Lando will block them. This can be done with Edit Related Revision on the right side of a phabricator page.

Ok, thanks for the advice. I reordered the commits locally, and resubmitted them with moz-phab, which I hoped did the adjustment of the parent-child-relationships automatically, but apparently it does not modify them if the revisions already exist on Phabricator.

Comment 45

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Remove code duplication in DatabaseOperationBase::BindKeyRangeToStatement overloads r=ttung

Depends on D41404

Comment 46

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Refactored building of key range and direction clauses. r=ttung

Depends on D42852

Comment 47

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/4f3b348afe01
https://hg.mozilla.org/mozilla-central/rev/26a290615bbd
https://hg.mozilla.org/mozilla-central/rev/f0f5d12cc2b7
https://hg.mozilla.org/mozilla-central/rev/fa95640baa86

Comment 48

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Make limit parameter a statement parameter. r=ttung

Depends on D42853

Comment 49

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Inline Key::Assert function to make effective use of MOZ_ASSERT diagnostics. r=ttung

Depends on D43038

Comment 50

[Simon Giesecke [:sg] [he/him]]

When implementing this, I found a problem with a web-platform-test here: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/IndexedDB/idbcursor_iterating.htm#54

This deletes the next record after the current cursor position via IDBObjectStore.delete, and expects that the cursor does not iterate to that deleted record. If that record has been preloaded, the cursor however does not take notice of that with the current implementation approach, and I don't see a way right now how to resolve this efficiently. If we need to check in the child for each record if it still exists, then preloading will become ineffective IMO. However, I am not sure if this web-platform-test is correct. Does the specification really imply that the cursor must take notice of this?

Note that this issue doesn't exist if IDBCursor.delete is called. In that case, we can invalidate the cached records, since this is local to the cursor object. (Apart from that probably not being necessary at all, since IDBCursor.delete can only delete the current record, but not a subsequent one.)

Comment 51

[Simon Giesecke [:sg] [he/him]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #50)

When implementing this, I found a problem with a web-platform-test here: https://searchfox.org/mozilla-central/source/testing/web-platform/tests/IndexedDB/idbcursor_iterating.htm#54

This deletes the next record after the current cursor position via IDBObjectStore.delete, and expects that the cursor does not iterate to that deleted record. If that record has been preloaded, the cursor however does not take notice of that with the current implementation approach, and I don't see a way right now how to resolve this efficiently. If we need to check in the child for each record if it still exists, then preloading will become ineffective IMO. However, I am not sure if this web-platform-test is correct. Does the specification really imply that the cursor must take notice of this?

Note that this issue doesn't exist if IDBCursor.delete is called. In that case, we can invalidate the cached records, since this is local to the cursor object. (Apart from that probably not being necessary at all, since IDBCursor.delete can only delete the current record, but not a subsequent one.)

I think I resolved this after discussion with :ytausky, at least for the particular case mentioned above: the delete call is done in the same transaction the cursor was opened from, and in that case, we can actually notify the cursor.

As Yaron pointed out, the spec says "It is possible for the list of records which the cursor is iterating over to change before the full range of the cursor has been iterated." and some more related things in the rest of the paragraph, but this can interpreted in different ways. One interpretation (and Yaron thought this is more likely) is that this states that the cursor indeed should observe such changes, at least in some cases (and the test referenced above materializes this). However, another interpretation would just be that it is a note to the implementor not to try to maintain the position as an index (which is indeed explicitly stated in the next sentence: "It is possible for the list of records which the cursor is iterating over to change before the full range of the cursor has been iterated."), which would be invalidated by deletion operation, and saying nothing about the observability of changes.

NB: I found the discussion leading to this wording, see https://www.w3.org/Bugs/Public/show_bug.cgi?id=10088 and the thread starting with http://lists.w3.org/Archives/Public/public-webapps/2010JulSep/0056.html

Following up on this, I found the statements in https://w3c.github.io/IndexedDB/#transaction-scheduling, which clarify the isolation of multiple transactions (while never using the term "isolation"). I am not sure which way suggested in the box after read-only transactions we take, however I haven't seen any mention of snapshots taken in the code, so I assume we don't do that.

Comment 52

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Apply renamings suggested for Cursor::*Key members and related identifiers. r=ttung

Depends on D43039

Comment 53

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Refactored tests, reduced duplicated code. r=ttung

Depends on D43249

Comment 54

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Allow current key to be provided with PBackgroundIDBCursor::Continue. r=ttung

The key is assumed to be unset for now.

Depends on D43250

Comment 55

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Send extra records with every ObjectStoreCursorResponse if enabled by pref. r=ttung

Depends on D43251

Comment 56

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Do not use memutils for nsTArray of IndexCursorResponse. r=ttung

Depends on D43252

Comment 57

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Support preloading also for index cursors. r=ttung

Depends on D43460

Comment 58

[Andrew Sutherland [:asuth] (he/him)]

Right, our implementation doesn't do snapshots. Per https://github.com/w3c/IndexedDB/issues/253 I think Chrome/Blink did implement snapshotting and they've now removed it (or are still planning to).

The conclusions in comment 51 seem right to me. Such mutations can only occur in a write transaction and a write transaction can only occur in a single global, allowing for synchronous access to all related interfaces exposed to content. Requests are serialized in terms of mutation (http://lists.w3.org/Archives/Public/public-webapps/2010JulSep/0063.html), which makes it feasible and appropriate for us to synchronously invalidate all preloaded results for all cursors belonging to the mutated object store or its indices that haven't already been "consumed" by the act of invoking continue() which posts a task with the consumed result.

Comment 59

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Introduce preference for maximum number of extra records to preload, defaulting to 0/off for now. r=ttung

Depends on D43461

Comment 60

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Increase default maximum number of extra records to preload to 64. r=ttung

Depends on D43466

Comment 61

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Avoid copying keys on cursor creation. r=ttung

Depends on D43467

Comment 62

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Use const where easily possible. r=ttung

Depends on D43470

Comment 63

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Add support for preloading key-only cursors. r=ttung

Depends on D43631

Comment 64

[Simon Giesecke [:sg] [he/him]]

I think by the existing patches I solved this mostly. One case that is not handled nicely on the child side is the use of ContinuePrimaryKey, which does not use the preloaded results, but invalidates all. I might open a separate bug to fix that.

However, one more critical thing that still needs to be solved is the ordering when there are multiple interleaved requests. Currently, two wpts are failing (large-requests-abort.html, request-event-ordering.html), complaining about wrong event ordering (https://treeherder.mozilla.org/#/jobs?repo=try&revision=b948a3fc9243a8ff46c3b96316117296bf188748&selectedJob=264203112). This appears to be trickier than I thought. Without preloading, the sequence was implicitly kept because each child request was processed by the parent, and it processed them in order. With preloading, some child requests are handled by the child only. While they are not handled synchronously, but the response is dispatched and the order is not automatically held. It must be postponed until the right place in the sequence of requests. However, at the moment, a request sequence number is only maintained for logging purposes (IDBRequest::LoggingSerialNumber), and I have the impression that it is not always correct at places where it was not used before.

Any advice how to deal with this is appreciated. In particular:

• Is it the right approach to check serial/sequence numbers to produce the response at the right time?
• Can the IDBRequest::LoggingSerialNumber be used for this purpose? (which should probably be renamed not to refer to Logging anymore in that case)

Comment 65

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Reduced duplication for handling sort key comparisons in SQL queries and improved type-safety. r=ttung

Depends on D44009

Comment 66

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Removed implicit conversion operator of CachedStatement.

Depends on D44377

Comment 67

[Simon Giesecke [:sg] [he/him]]

This is the commit that introduced the ordering tests to web-platform-tests: https://github.com/web-platform-tests/wpt/commit/a414c1dd8ea33b9884ea87d13bd493629b640b44. It has a rather long description, but the main purpose of this is related to blob serialization, and I don't oversee in detail how this relates to the requirement for request ordering.

Comment 68

[Andrew Sutherland [:asuth] (he/him)]

Okay, so AIUI (as I understand it) https://w3c.github.io/IndexedDB/#transaction-lifetime in step 2 explicitly does call out that requests must be processed in order, so the test is currently right.

Approach-wise, the simplest approach if of course just to invalidate if there's ever a request issued that isn't a consecutive cursor operation. But that seems likely to induce prefetch misses that would have been prefetch hits if we could delay the response. This might need/want a serial number mechanism anyways? If you're okay with the complexity for this stack, I think it would certainly be desirable to pursue delaying the results appropriately.

I think we can repurpose LoggingSerialNumber, yes. It seems like we issue it right now off of the weird ThreadLocal thing at https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/dom/indexedDB/ActorsChild.h#59 from a per-thread one-up counter. We also have one-up counters for transaction serial numbers and version change transaction serial numbers. I guess we can leave transaction numbers as-is, but I'm fine with any/all overhauls here as long as we ensure that the IDB_LOG invocations still create a unique tuple in what they log over thread identifiers, pointers logged, and the various transaction/request id's. We can add and log an id on the database if needed.

Comment 69

[Simon Giesecke [:sg] [he/him]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #68)

Okay, so AIUI (as I understand it) https://w3c.github.io/IndexedDB/#transaction-lifetime in step 2 explicitly does call out that requests must be processed in order, so the test is currently right.

Right, it is very explicit about that.

Approach-wise, the simplest approach if of course just to invalidate if there's ever a request issued that isn't a consecutive cursor operation.
I am just becoming unsure if there might be a missing invalidation. I already invalidate the cached results in some cases that could affect their validity. However, this might not solve the problem if the invalidation is refined more, and only done conditionally for cursors that might actually be affected, since the change affects their remaining range. Currently, the invalidation does not check that.

But that seems likely to induce prefetch misses that would have been prefetch hits if we could delay the response. This might need/want a serial number mechanism anyways? If you're okay with the complexity for this stack, I think it would certainly be desirable to pursue delaying the results appropriately.

As said above, maybe currently there is only an invalidation missing. In that case, I might only add that now, and postpone the serial number issue to a separate bug that will introduce conditional invalidation as mentioned above. Otherwise, I will do it as part of this bug.

I think we can repurpose LoggingSerialNumber, yes. It seems like we issue it right now off of the weird ThreadLocal thing at https://searchfox.org/mozilla-central/rev/e5327b05c822cdac24e233afa37d72c0552dbbaf/dom/indexedDB/ActorsChild.h#59 from a per-thread one-up counter. We also have one-up counters for transaction serial numbers and version change transaction serial numbers. I guess we can leave transaction numbers as-is, but I'm fine with any/all overhauls here as long as we ensure that the IDB_LOG invocations still create a unique tuple in what they log over thread identifiers, pointers logged, and the various transaction/request id's. We can add and log an id on the database if needed.

Ok, thanks for the judgment!

Comment 70

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Replaced custom for loops by range-based for or STL algorithms. r=ytausky

Depends on D44443

Comment 71

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Extract functions for accessing sort key into Cursor. r=ytausky

Depends on D44988

Comment 72

[Simon Giesecke [:sg] [he/him]]

Please checkin D41403, D42645, D40954, D40955, D40956

Comment 73

[Simon Giesecke [:sg] [he/him]]

Also D41025, D41026 and D41225

Comment 74

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Extract common function for discarding cached responses. r=ytausky

Depends on D45308

Comment 75

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e72ef41a4a78
Added documentation on Cursor data members. r=ttung
https://hg.mozilla.org/integration/mozilla-inbound/rev/07cafa88a5b1
Added comments on almost-const data members r=ttung
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e46dad972c8
Reduce the use of plain pointers, and use const for Cursor data members where possible. r=ttung
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6aa1c5bc7d9
Change definition of OpenCursorParams subtypes to allow reducing duplicated code r=ttung
https://hg.mozilla.org/integration/mozilla-inbound/rev/300dc3034eeb
Reduce code duplication for different open cursor variants. r=ttung
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c38ead5e55
Extracted methods Cursor::OpenOp::PrepareKeyConditionClauses and PrepareIndexKeyConditionClause. r=ttung
https://hg.mozilla.org/integration/mozilla-inbound/rev/eff806e9615d
Unified handling of literal strings using NS_LITERAL_(C)STRING and removed some duplications. r=ttung
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5809ccdc7d2
Extract DatabaseConnection::ExecuteCachedStatement function. r=ttung

Comment 76

[Alexandru Michis [:malexandru]]

Backed out 8 changesets for causing failures in DatabaseConnection::CachedStatement function.

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c22109981db288df076212551147c9e050565e3

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&tochange=7c22109981db288df076212551147c9e050565e3&fromchange=a5809ccdc7d225f326359350494cd62bac3eed9a

Failure logs:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=265936275&repo=mozilla-inbound&lineNumber=1478
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=265936398&repo=mozilla-inbound&lineNumber=1474

[task 2019-09-10T15:23:04.350Z] 15:23:04 INFO - Assertion failure: mStatement, at /builds/worker/workspace/build/src/dom/indexedDB/ActorsParent.cpp:10086
[task 2019-09-10T15:23:04.351Z] 15:23:04 INFO - Assertion failure: mStatement, at /builds/worker/workspace/build/src/dom/indexedDB/ActorsParent.cpp:10086

[task 2019-09-10T15:22:31.605Z] 15:22:31 INFO - TEST-START | /animation-worklet/animation-worklet-inside-iframe.https.html
[task 2019-09-10T15:22:31.607Z] 15:22:31 INFO - Closing window 14
[task 2019-09-10T15:22:32.528Z] 15:22:32 INFO - IOError on command, setting status to CRASH
[task 2019-09-10T15:22:33.891Z] 15:22:33 INFO - mozcrash Copy/paste: /builds/worker/workspace/build/linux64-minidump_stackwalk /tmp/tmpjq9cwD/475fa87d-291d-37e4-f4f6-1f3078823238.dmp /builds/worker/workspace/build/symbols
[task 2019-09-10T15:22:39.497Z] 15:22:39 INFO - mozcrash Saved minidump as /builds/worker/workspace/build/blobber_upload_dir/475fa87d-291d-37e4-f4f6-1f3078823238.dmp
[task 2019-09-10T15:22:39.497Z] 15:22:39 INFO - mozcrash Saved app info as /builds/worker/workspace/build/blobber_upload_dir/475fa87d-291d-37e4-f4f6-1f3078823238.extra
[task 2019-09-10T15:22:39.616Z] 15:22:39 INFO - PROCESS-CRASH | /animation-worklet/animation-worklet-inside-iframe.https.html | application crashed [@ mozilla::dom::indexedDB::(anonymous namespace)::DatabaseConnection::CachedStatement::operator mozIStorageStatement*() const]
[task 2019-09-10T15:22:39.616Z] 15:22:39 INFO - Crash dump filename: /tmp/tmpjq9cwD/475fa87d-291d-37e4-f4f6-1f3078823238.dmp
[task 2019-09-10T15:22:39.616Z] 15:22:39 INFO - Operating system: Android
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - 0.0.0 Linux 3.10.0+ #260 SMP PREEMPT Fri May 19 12:48:14 PDT 2017 x86_64
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - CPU: amd64
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - family 6 model 6 stepping 3
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - 4 CPUs
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO -
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - GPU: UNKNOWN
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO -
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - Crash reason: SIGSEGV /SEGV_MAPERR
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - Crash address: 0x0
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - Process uptime: not available
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO -
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - Thread 42 (crashed)
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - 0 libxul.so!mozilla::dom::indexedDB::(anonymous namespace)::DatabaseConnection::CachedStatement::operator mozIStorageStatement*() const [ActorsParent.cpp:a5809ccdc7d225f326359350494cd62bac3eed9a : 10086 + 0x29]
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - rax = 0x00007e298609e313 rdx = 0x0000000000000004
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - rcx = 0x00007e2988df2ab0 rbx = 0x00007e298897a590
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - rsi = 0x00007e2988979dc0 rdi = 0x000000000000001b
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - rbp = 0x00007e298897a480 rsp = 0x00007e298897a470
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - r8 = 0x0000000000000000 r9 = 0x00007e29a5a2a090
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - r10 = 0x0000000000000013 r11 = 0x0000000000000246
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - r12 = 0x00007e29763c6eb0 r13 = 0x0000000000000002
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - r14 = 0x00007e298897a50c r15 = 0x00007e298897a590
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - rip = 0x00007e29820990f8
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - Found by: given as instruction pointer in context
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - 1 libxul.so!mozilla::dom::indexedDB::(anonymous namespace)::DatabaseConnection::GetFreelistCount(mozilla::dom::indexedDB::(anonymous namespace)::DatabaseConnection::CachedStatement&, unsigned int*) [ActorsParent.cpp:a5809ccdc7d225f326359350494cd62bac3eed9a : 9925 + 0x8]
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - rbx = 0x00007e298897a4a0 rbp = 0x00007e298897a4f0
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - rsp = 0x00007e298897a490 r12 = 0x00007e29763c6eb0
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - r13 = 0x0000000000000002 r14 = 0x00007e298897a50c
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - r15 = 0x00007e298897a590 rip = 0x00007e2982098806
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - Found by: call frame info
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - 2 libxul.so!mozilla::dom::indexedDB::(anonymous namespace)::DatabaseConnection::DoIdleProcessing(bool) [ActorsParent.cpp:a5809ccdc7d225f326359350494cd62bac3eed9a : 9744 + 0xb]
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - rbx = 0x00007e298897a590 rbp = 0x00007e298897a6a0
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - rsp = 0x00007e298897a500 r12 = 0x00007e2976324f10
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - r13 = 0x0000000000000002 r14 = 0x0000000000000001
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - r15 = 0x00007e29763c6eb0 rip = 0x00007e2982097a9d
[task 2019-09-10T15:22:39.617Z] 15:22:39 INFO - Found by: call frame info
[task 2019-09-10T15:22:39.618Z] 15:22:39 INFO - 3 libxul.so!mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::IdleConnectionRunnable::Run() [ActorsParent.cpp:a5809ccdc7d225f326359350494cd62bac3eed9a : 11717 + 0x11]

Comment 77

[Simon Giesecke [:sg] [he/him]]

I was able to reproduce the crash, however only after rebasing. I will investigate which intermediate changes cause this.

Comment 78

[Simon Giesecke [:sg] [he/him]]

I hopefully fixed this, please land D41403, D42645, D40954, D40955, D40956, D41025, D41026 and D41225 again.

Comment 79

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0133605c6169
Added documentation on Cursor data members. r=asuth
https://hg.mozilla.org/integration/autoland/rev/acae76cea927
Added comments on almost-const data members r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/f5aee1ae7282
Reduce the use of plain pointers, and use const for Cursor data members where possible. r=ttung
https://hg.mozilla.org/integration/autoland/rev/6078d5a9c010
Change definition of OpenCursorParams subtypes to allow reducing duplicated code r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/5be5a3bf0499
Reduce code duplication for different open cursor variants. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/4858e4e132de
Extracted methods Cursor::OpenOp::PrepareKeyConditionClauses and PrepareIndexKeyConditionClause. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/1485cea56f41
Unified handling of literal strings using NS_LITERAL_(C)STRING and removed some duplications. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/066bee573945
Extract DatabaseConnection::ExecuteCachedStatement function. r=ytausky,asuth

Comment 80

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Replace custom for loops over hashtables by range-based for/STL algorithms.

Comment 81

[Alexandru Michis [:malexandru]]

Backed out 8 changesets for failures in idbcursor-continuePrimaryKey.htm

Backout link: https://hg.mozilla.org/integration/autoland/rev/e78f7880e00a07add5028fc0bb4e860151907dd7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=266090246&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&tochange=e78f7880e00a07add5028fc0bb4e860151907dd7&fromchange=066bee5739450f5f62f64e22da65611a108eeea5&searchStr=wpt18

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266092823&repo=autoland&lineNumber=14042

[task 2019-09-11T11:01:24.811Z] 11:01:24 INFO - PID 5132 | ++DOMWINDOW == 4 (000001FEEB890000) [pid = 10624] [serial = 4] [outer = 000001FEEB812020]
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO -
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - TEST-UNEXPECTED-FAIL | /IndexedDB/idbcursor-continuePrimaryKey.htm | IndexedDB: IDBCursor method continuePrimaryKey() - cursor is null
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - verifyContinueCalls/request.onsuccess<@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:113:15
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:109:31
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:103:28
[task 2019-09-11T11:01:25.022Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:103:28
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:103:28
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:103:28
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - EventHandlerNonNullverifyContinueCalls@http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:103:28
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - @http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:132:7
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - indexeddb_test/</open.onsuccess<@http://web-platform.test:8000/IndexedDB/support.js:131:20
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step_func/<@http://web-platform.test:8000/resources/testharness.js:1635:35
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - EventHandlerNonNull*indexeddb_test/<@http://web-platform.test:8000/IndexedDB/support.js:128:26
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1611:25
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - async_test@http://web-platform.test:8000/resources/testharness.js:576:22
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - indexeddb_test@http://web-platform.test:8000/IndexedDB/support.js:105:13
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - @http://web-platform.test:8000/IndexedDB/idbcursor-continuePrimaryKey.htm:13:15
[task 2019-09-11T11:01:25.023Z] 11:01:25 INFO - TEST-OK | /IndexedDB/idbcursor-continuePrimaryKey.htm | took 511ms

Comment 82

[Simon Giesecke [:sg] [he/him]]

Fixed the issue in idbcursor-continuePrimaryKey.htm. Please land D41403, D42645, D40954, D40955, D40956, D41025, D41026 and D41225 again.

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96ab87dbe78f
Added documentation on Cursor data members. r=asuth
https://hg.mozilla.org/integration/autoland/rev/f25d82f6f518
Added comments on almost-const data members r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/556059030dc6
Reduce the use of plain pointers, and use const for Cursor data members where possible. r=ttung
https://hg.mozilla.org/integration/autoland/rev/0e77abe58616
Change definition of OpenCursorParams subtypes to allow reducing duplicated code r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/23fc7469065d
Reduce code duplication for different open cursor variants. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/fb5e8aa853b0
Extracted methods Cursor::OpenOp::PrepareKeyConditionClauses and PrepareIndexKeyConditionClause. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/f951fbdc5160
Unified handling of literal strings using NS_LITERAL_(C)STRING and removed some duplications. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/d1ce54f71152
Extract DatabaseConnection::ExecuteCachedStatement function. r=ytausky,asuth

Comment 84

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/96ab87dbe78f
https://hg.mozilla.org/mozilla-central/rev/f25d82f6f518
https://hg.mozilla.org/mozilla-central/rev/556059030dc6
https://hg.mozilla.org/mozilla-central/rev/0e77abe58616
https://hg.mozilla.org/mozilla-central/rev/23fc7469065d
https://hg.mozilla.org/mozilla-central/rev/fb5e8aa853b0
https://hg.mozilla.org/mozilla-central/rev/f951fbdc5160
https://hg.mozilla.org/mozilla-central/rev/d1ce54f71152

Comment 85

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Removed duplication between different BackgroundCursorChild::HandleResponse overloads. r=ttung

Depends on D45506

Comment 86

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Do not use mContinueCalled for determining whether to cache a cursor response. r=ttung

Depends on D45674

Comment 87

[Simon Giesecke [:sg] [he/him]]

Please land patches D41404, D42852, D42853, D43038, D43039, D43249, D43250

Comment 88

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65938107d86c
Move the functionality of OpenOp::GetKeyRangeInfo to Cursor::SetOptionalKeyRange r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/dc5a09a61450
Remove code duplication in DatabaseOperationBase::BindKeyRangeToStatement overloads r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/96ced3a647df
Refactored building of key range and direction clauses. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/8cc54010f8a2
Make limit parameter a statement parameter. r=ytausky,asuth
https://hg.mozilla.org/integration/autoland/rev/714d36c43204
Inline Key::Assert function to make effective use of MOZ_ASSERT diagnostics. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/9966f5d71bb2
Apply renamings suggested for Cursor::*Key members and related identifiers. r=ytausky,asuth
https://hg.mozilla.org/integration/autoland/rev/d7a6473d622e
Refactored tests, reduced duplicated code. r=ttung,asuth

Comment 89

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/65938107d86c
https://hg.mozilla.org/mozilla-central/rev/dc5a09a61450
https://hg.mozilla.org/mozilla-central/rev/96ced3a647df
https://hg.mozilla.org/mozilla-central/rev/8cc54010f8a2
https://hg.mozilla.org/mozilla-central/rev/714d36c43204
https://hg.mozilla.org/mozilla-central/rev/9966f5d71bb2
https://hg.mozilla.org/mozilla-central/rev/d7a6473d622e

Comment 90

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Remove duplication in uses of IDB_LOG_MARK. r=ttung

Comment 91

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Replace pseudo-move constructor of StructuredCloneReadInfo by explicit DeserializeStructuredCloneReadInfo function. r=ttung

Depends on D45675

Comment 92

[James Jahns]

Attachment: Backed out 8 changesets (Bug 1168606) for failures in idbcursor-continuePrimaryKey.htm

Backed out changeset 066bee573945 (bug 1168606)
Backed out changeset 1485cea56f41 (bug 1168606)
Backed out changeset 4858e4e132de (bug 1168606)
Backed out changeset 5be5a3bf0499 (bug 1168606)
Backed out changeset 6078d5a9c010 (bug 1168606)
Backed out changeset f5aee1ae7282 (bug 1168606)
Backed out changeset acae76cea927 (bug 1168606)
Backed out changeset 0133605c6169 (bug 1168606)

Comment 93

[Simon Giesecke [:sg] [he/him]]

James, what does this mean? There are no details for the reason of the backout, the Phabricator revisions were not reopened, and I was not needinfo-ed. Is this referring to the issue already mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1168606#c81? I think I resolved that. I am not sure what to do now.

Comment 94

[James Jahns]

(In reply to Simon Giesecke [:sg] [he/him] from comment #93)

James, what does this mean? There are no details for the reason of the backout, the Phabricator revisions were not reopened, and I was not needinfo-ed. Is this referring to the issue already mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1168606#c81? I think I resolved that. I am not sure what to do now.

Hi, my apologies. I made some mistakes with Mercurial and ending up submitting the wrong patch, so this is totally in error... please ignore.

Comment 95

[Simon Giesecke [:sg] [he/him]]

Please check-in D46576 and D43251

Comment 96

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/340dfa69aed8
Remove duplication in uses of IDB_LOG_MARK. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/d2772a7482ed
Allow current key to be provided with PBackgroundIDBCursor::Continue. r=ttung,asuth

Comment 97

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/340dfa69aed8
https://hg.mozilla.org/mozilla-central/rev/d2772a7482ed

Comment 98

[Simon Giesecke [:sg] [he/him]]

Please check-in D43470, D43631, D44988 and D43460

Comment 99

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e493a6b5f125
Avoid copying keys on cursor creation. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/9e7a90dcc429
Use const where easily possible. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/f844afbebc6a
Replaced custom for loops by range-based for or STL algorithms. r=ttung
https://hg.mozilla.org/integration/autoland/rev/63cf0966cb41
Do not use memutils for nsTArray of IndexCursorResponse. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/fc5e1bee1332
Send two records with every ObjectStoreCursorResponse. r=ttung,asuth

Comment 100

[Cristian Brindusan [:cbrindusan]]

Backed out 5 changesets (Bug 1168606) for xpcshell failures in dom/indexedDB/test/unit/test_temporary_storage.js

Backout: https://hg.mozilla.org/integration/autoland/rev/e7e97d7a6a43d7012dc5517b088122bb7b543a65

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=fc5e1bee13321946704a915ef4c7c80c0941dcb3&selectedJob=269311913

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=269311913&repo=autoland&lineNumber=2639

Comment 101

[Simon Giesecke [:sg] [he/him]]

I fixed the issue with xpcshell tests on Android, and the patches should be ready for landing now.

Comment 102

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://phabricator.services.mozilla.com/D43631 has a conflict:

applying /tmp/tmp3LcxUW dom/indexedDB/ActorsParent.cpp Hunk #26 FAILED at 16475. 1 out of 62 hunks FAILED -- saving rejects to file dom/indexedDB/ActorsParent.cpp.rej abort: patch command failed: exited with status 256

https://hg.mozilla.org/mozilla-central/rev/ea140aa9122543c1964c4641cb566d168e127022#l1.169 conflicted.

Comment 103

[Simon Giesecke [:sg] [he/him]]

I rebased the patches to current mozilla-central.

I ran tests at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=05e33e6d918f35d3b3f84290d5fb17ee9c2beb75

D43470, D43631, D44988 and D43460 should be ready now.

Comment 104

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc11a47c3ea7
Avoid copying keys on cursor creation. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/cddcfdcb52f6
Use const where easily possible. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/acf2d5bca8b1
Replaced custom for loops by range-based for or STL algorithms. r=ttung
https://hg.mozilla.org/integration/autoland/rev/ad5ebdad4096
Do not use memutils for nsTArray of IndexCursorResponse. r=froydnj

Comment 105

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/fc11a47c3ea7
https://hg.mozilla.org/mozilla-central/rev/cddcfdcb52f6
https://hg.mozilla.org/mozilla-central/rev/acf2d5bca8b1
https://hg.mozilla.org/mozilla-central/rev/ad5ebdad4096

Comment 106

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Added test case for IDBObjectStore.delete while iterating a cursor. r=asuth

Comment 107

[Andrew Sutherland [:asuth] (he/him)]

NI on myself to look deeper into the https://phabricator.services.mozilla.com/D48504 test case.

Comment 108

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Resolved clang-tidy warnings. r=ttung

Depends on D48504

Comment 109

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Fixed some log messages. r=ttung

Depends on D46593

Comment 110

[Andrew Creskey [:acreskey]]

I'm seeing a crash on OSX local builds of mozilla-central.
It ends in a EXC_BAD_ACCESS from mozilla::dom::indexedDB::(anonymous namespace)::Cursor::OpenOp::DoDatabaseWork(this=0x000000012ca69500, aConnection=0x000000012ca7bb80)::DatabaseConnection*) at ActorsParent.cpp:26316

https://paste.rs/0Y7

So far it looks like I'm the only person seeing this.

Comment 111

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Fix code generation issue on Mac OS X. r=ttung

As reported in https://bugzilla.mozilla.org/show_bug.cgi?id=1168606#c110,
some Mac OS X release build crashes with the original code, probably due
to some temporary string being destroyed prematurely. Assigning to a
named variable reportedly solves the issue.

Comment 112

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c83949ce2908
Fix code generation issue on Mac OS X. r=acreskey,ttung

Comment 113

[Natalia Csoregi [:nataliaCs]]

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

Comment 114

[Simon Giesecke [:sg] [he/him]]

A Pernosco trace for the https://phabricator.services.mozilla.com/D48504 test case is now at https://pernos.co/debug/xIn5BlmpbBNwDwZyvOQ3Fg/index.html

Comment 115

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d8b02674d45
Introduce preference for maximum number of extra records to preload, defaulting to 0/off for now. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/8dc185488801
Send extra records with every ObjectStoreCursorResponse if enabled by pref. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/f1be81af7d6b
Support preloading also for index cursors. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/6818224b9b11
Add support for preloading key-only cursors. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/96cb7140a4f6
Reduced duplication for handling sort key comparisons in SQL queries and improved type-safety. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/41f1bef0f019
Removed implicit conversion operator of CachedStatement. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/dffe1e6c58bd
Extract functions for accessing sort key into Cursor. r=ytausky,asuth
https://hg.mozilla.org/integration/autoland/rev/8c776a5cb5db
Extract common function for discarding cached responses. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/918dc7cce5e9
Removed duplication between different BackgroundCursorChild::HandleResponse overloads. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/8fdc4a3d3ecb
Do not use mContinueCalled for determining whether to cache a cursor response. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/a2ae085f929f
Fixed some log messages. r=ttung

Comment 116

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/307bd8d24407
Fix build bustages on ActorsParent.cpp. r=sgiesecke CLOSED TREE

Comment 117

[Cosmin Sabou [:CosminS]]

Backed out for causing build bustages on ActorsParent.cpp and Assertions.h.

Pushes with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=a2ae085f929f99c56ce147e21950dc0eb982583e&selectedJob=274622737

https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=307bd8d244079e2ee96773b93938f8d556041211&selectedJob=274628664

Failure logs: https://treeherder.mozilla.org/logviewer.html#?job_id=274622737&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=274628664&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/2172f9a7aaf042b10bc1c789f7fea97fc50ba0ef

Comment 118

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f0447b228c7
Introduce preference for maximum number of extra records to preload, defaulting to 0/off for now. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/89817a19f3b3
Send extra records with every ObjectStoreCursorResponse if enabled by pref. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/c009bbdbbfd5
Support preloading also for index cursors. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/b525ad6838f0
Add support for preloading key-only cursors. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/cf53f4e57313
Reduced duplication for handling sort key comparisons in SQL queries and improved type-safety. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/1abd3e7f9df4
Removed implicit conversion operator of CachedStatement. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/9eeb9fd126bc
Extract functions for accessing sort key into Cursor. r=ytausky,asuth
https://hg.mozilla.org/integration/autoland/rev/c6d3254dcd8e
Extract common function for discarding cached responses. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/61c29e7a5419
Removed duplication between different BackgroundCursorChild::HandleResponse overloads. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/bd872bd17560
Do not use mContinueCalled for determining whether to cache a cursor response. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/c33c9703069b
Fixed some log messages. r=ttung

Comment 119

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/2f0447b228c7
https://hg.mozilla.org/mozilla-central/rev/89817a19f3b3
https://hg.mozilla.org/mozilla-central/rev/c009bbdbbfd5
https://hg.mozilla.org/mozilla-central/rev/b525ad6838f0
https://hg.mozilla.org/mozilla-central/rev/cf53f4e57313
https://hg.mozilla.org/mozilla-central/rev/1abd3e7f9df4
https://hg.mozilla.org/mozilla-central/rev/9eeb9fd126bc
https://hg.mozilla.org/mozilla-central/rev/c6d3254dcd8e
https://hg.mozilla.org/mozilla-central/rev/61c29e7a5419
https://hg.mozilla.org/mozilla-central/rev/bd872bd17560
https://hg.mozilla.org/mozilla-central/rev/c33c9703069b

Comment 120

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Changes to test to induce failure case discussed.

This applies to https://phabricator.services.mozilla.com/D48504

Comment 121

[Andrew Sutherland [:asuth] (he/him)]

Attachment: MOZ_LOG=IndexedDB:5 for the test failure when running with the test.

Feel free to obsolete this log and the change patch when addressed.

Comment 122

[Andrew Sutherland [:asuth] (he/him)]

And to be super clear, we should only enable the (amazing, awesome, altogether massive performance enhancement) preloading feature once that test passes.

Comment 123

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Fix code issue, use appropriate MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE macro. r=ttung

Depends on D49274

Comment 124

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/43dbad71664b
Replace pseudo-move constructor of StructuredCloneReadInfo by explicit DeserializeStructuredCloneReadInfo function. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/a36036d07b9f
Replace custom for loops over hashtables by range-based for/STL algorithms. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/69a0758705a9
Resolved clang-tidy warnings. r=ttung,asuth

Comment 125

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/43dbad71664b
https://hg.mozilla.org/mozilla-central/rev/a36036d07b9f
https://hg.mozilla.org/mozilla-central/rev/69a0758705a9

Comment 126

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1168606 - Set flag to invalidate in-flight responses as well. r=asuth

Comment 127

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe3a07722fee
Increase default maximum number of extra records to preload to 64. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/fc34f807986e
Added test case for IDBObjectStore.delete while iterating a cursor. r=asuth
https://hg.mozilla.org/integration/autoland/rev/1ca7e5b24181
Set flag to invalidate in-flight responses as well. r=asuth

Comment 128

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/20252 for changes under testing/web-platform/tests

Comment 129

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 130

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/fe3a07722fee
https://hg.mozilla.org/mozilla-central/rev/fc34f807986e
https://hg.mozilla.org/mozilla-central/rev/1ca7e5b24181

Comment 131

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot