Closed Bug 1168606 Opened 5 years ago Closed 3 days ago

Implement preloading cursors for IndexedDB

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox41 --- affected
firefox72 --- fixed

People

(Reporter: poiru, Assigned: sg)

References

(Blocks 4 open bugs, Regressed 1 open bug)

Details

(Whiteboard: DWS_NEXT [qf:p2], [wptsync upstream])

Attachments

(50 files, 5 obsolete files)

12.44 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
4.08 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
15.35 KB, patch
khuey
: review+
Details | Diff | Splinter Review
7.19 KB, patch
khuey
: review+
Details | Diff | Splinter Review
12.61 KB, patch
khuey
: feedback+
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
4.33 KB, patch
Details | Diff | Splinter Review
173.36 KB, text/plain
Details
47 bytes, text/x-phabricator-request
Details | Review
Cursors should be able to preload records in the background as needed rather than loading everything at once.
This be used for a delayed continue runnable later on.
Attachment #8617357 - Flags: review?(bent.mozilla)
For now, we assume that it always carries a single record.
Attachment #8621233 - Flags: review?(bent.mozilla)
This is in preparation for a future patch that will include the current key in
the request.
Attachment #8621235 - Flags: review?(bent.mozilla)
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;
  }
Attachment #8617354 - Flags: review?(bent.mozilla) → review+
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?
Attachment #8617357 - Flags: review?(bent.mozilla) → review-
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[];
  };
Attachment #8621233 - Flags: review?(bent.mozilla) → review-
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);
Attachment #8621235 - Flags: review?(bent.mozilla) → review-
(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...)
Attachment #8621233 - Attachment is obsolete: true
Attachment #8627381 - Flags: review?(bent.mozilla)
(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.
Attachment #8621235 - Attachment is obsolete: true
Attachment #8627382 - Flags: review?(bent.mozilla)
(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?
Flags: needinfo?(bent.mozilla)
(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.
Flags: needinfo?(bent.mozilla)
Attachment #8627381 - Flags: review?(bent.mozilla) → review?(khuey)
Attachment #8627382 - Flags: review?(bent.mozilla) → review?(khuey)
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 ...
Attachment #8627381 - Flags: review?(khuey) → review+
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.
Attachment #8639389 - Flags: feedback?(khuey)
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.
Attachment #8639389 - Flags: feedback?(khuey) → feedback+
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"?
Flags: needinfo?(birunthan)
(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.
Flags: needinfo?(birunthan)
Please open a new bug for any remaining work.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Reopen this bug because patch#4 & patch#5 are dead codes as explain in bug 1289375 comment 0.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: birunthan → nobody
Priority: -- → P3
Whiteboard: DWS_NEXT
See Also: → 1499550
Whiteboard: DWS_NEXT → DWS_NEXT [qf]
Priority: P3 → P2
Whiteboard: DWS_NEXT [qf] → DWS_NEXT [qf:p2]
Assignee: nobody → sgiesecke
Type: defect → task
Blocks: 1068232
Status: REOPENED → ASSIGNED

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

Depends on D40957

Also reduced duplication within the extracted methods.

Depends on D41024

Attachment #9083597 - Attachment description: Bug 1168606 - Reduce the use of plain pointers. r=ttung → Bug 1168606 - Reduce the use of plain pointers, and use const for Cursor data members where possible. r=ttung
Attachment #9084344 - Attachment description: Bug 1168606 - Modified GetRangeKeyInfo to avoid unncessary copying of the non-locale-based key in case the locale-based is needed, and reduced copde duplication and variable scoping. r=ttung → Bug 1168606 - Modified GetRangeKeyInfo to avoid unnecessary copying of the non-locale-based key in case the locale-based is needed, and reduced code duplication and variable scoping. r=ttung
Attachment #9084344 - Attachment description: Bug 1168606 - Modified GetRangeKeyInfo to avoid unnecessary copying of the non-locale-based key in case the locale-based is needed, and reduced code duplication and variable scoping. r=ttung → Bug 1168606 - Move the functionality of OpenOp::GetKeyRangeInfo to Cursor::SetOptionalKeyRange
Attachment #9084344 - Attachment description: Bug 1168606 - Move the functionality of OpenOp::GetKeyRangeInfo to Cursor::SetOptionalKeyRange → Bug 1168606 - Move the functionality of OpenOp::GetKeyRangeInfo to Cursor::SetOptionalKeyRange r=ttung

Please land revisions D40953, D40957, D41024 and D41191.

Do not land D41403 yet, it requires changes.

The other revisions have not yet completed review.

Pushed by archaeopteryx@coole-files.de:
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

Keywords: checkin-needed

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.

(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.

Attachment #9083982 - Attachment is obsolete: true

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.)

Flags: needinfo?(bugmail)

(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.

The key is assumed to be unset for now.

Depends on D43250

Blocks: 1576573

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.

Flags: needinfo?(bugmail)

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)
Flags: needinfo?(bugmail)
Attachment #9089115 - Attachment description: Bug 1168606 - Add spuport for preloading key-only cursors. r=ttung → Bug 1168606 - Add support for preloading key-only cursors. r=ttung

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.

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.

Flags: needinfo?(bugmail)

(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!

Please checkin D41403, D42645, D40954, D40955, D40956

Keywords: checkin-needed

Also D41025, D41026 and D41225

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

Pushed by malexandru@mozilla.com:
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

Keywords: checkin-needed

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]

Flags: needinfo?(sgiesecke)

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

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

Flags: needinfo?(sgiesecke)
Keywords: checkin-needed

Pushed by malexandru@mozilla.com:
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

Keywords: checkin-needed

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

Flags: needinfo?(sgiesecke)

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

Flags: needinfo?(sgiesecke)
Keywords: checkin-needed

Pushed by malexandru@mozilla.com:
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

Keywords: checkin-needed
Blocks: 1580499
Attachment #9087029 - Attachment description: Bug 1168606 - Refactored building of key range and direction clauses r=ttung → Bug 1168606 - Refactored building of key range and direction clauses. r=ttung

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

Keywords: checkin-needed

Pushed by apavel@mozilla.com:
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

Keywords: checkin-needed

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)

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.

Flags: needinfo?(jahnsjam)
Attachment #9096796 - Attachment is obsolete: true

(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.

Flags: needinfo?(jahnsjam)

Please check-in D46576 and D43251

Keywords: checkin-needed

Pushed by csabou@mozilla.com:
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

Keywords: checkin-needed

Please check-in D43470, D43631, D44988 and D43460

Keywords: checkin-needed

Pushed by dvarga@mozilla.com:
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

Keywords: checkin-needed

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

Flags: needinfo?(sgiesecke)
Keywords: checkin-needed

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.

Flags: needinfo?(sgiesecke)

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.

Flags: needinfo?(sgiesecke)
Keywords: checkin-needed

Pushed by apavel@mozilla.com:
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

Keywords: checkin-needed

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

Flags: needinfo?(bugmail)

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.

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.

Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c83949ce2908
Fix code generation issue on Mac OS X. r=acreskey,ttung
Attachment #9088163 - Attachment description: Bug 1168606 - Introduce preference for maximum number of extra records to preload. r=ttung → Bug 1168606 - Introduce preference for maximum number of extra records to preload, defaulting to 0/off for now. r=ttung
Attachment #9087740 - Attachment description: Bug 1168606 - Send two records with every ObjectStoreCursorResponse. r=ttung → Bug 1168606 - Send extra records with every ObjectStoreCursorResponse if enabled by pref. r=ttung
Pushed by sgiesecke@mozilla.com:
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
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/307bd8d24407
Fix build bustages on ActorsParent.cpp. r=sgiesecke CLOSED TREE
Pushed by sgiesecke@mozilla.com:
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
Flags: needinfo?(sgiesecke)

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

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

Attachment #9107497 - Attachment is obsolete: true
Pushed by sgiesecke@mozilla.com:
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
Keywords: leave-open
Pushed by sgiesecke@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/20252 for changes under testing/web-platform/tests
Whiteboard: DWS_NEXT [qf:p2] → DWS_NEXT [qf:p2], [wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago3 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Upstream PR merged by moz-wptsync-bot
Regressions: 1597038
You need to log in before you can comment on or make changes to this bug.