Closed Bug 1565224 Opened 5 years ago Closed 4 years ago

There should be some gtests for Key

Categories

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

Product:
Core
Component:
Storage: IndexedDB
Type:
task

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: sg, Assigned: sg, Mentored)

Details

Attachments

(3 files)

Bug 1565224 - Added some initial gtests for dom/indexedDB/Key. r=ttung
47 bytes, text/x-phabricator-request
Details | Review
Bug 1565224 - Added tests for Key::Set* and Key::CompareKeys. r=ttung
47 bytes, text/x-phabricator-request
Details | Review
Bug 1565224 - Re-enable Key gtests and fix usage of JSAPI
47 bytes, text/x-phabricator-request
Details | Review

Currently, there are no gtests for IndexedDB. While for most aspects, JS based tests are appropriate, some things are not exposed to JS and could better be tested on the C++ level. This includes tests for the encoding/decoding of keys. Such tests should be added in the context of this bug.

P3 for now.

Priority: -- → P5
Priority: P5 → P3
Attachment #9078757 - Attachment description: Bug 1565224 - Added some initials gtests for dom/indexedDB/Key r=ttung → Bug 1565224 - Added some initial gtests for dom/indexedDB/Key r=ttung
Keywords: leave-open

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:sg, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(sgiesecke)

The patches did not land yet because they suffer from rooting hazards as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1565224#c4. Unfortunately, I haven't been able yet to give those a look.

Flags: needinfo?(sgiesecke)

I finally found some time now to look at the rooting hazards, however I have no idea how to resolve them. I get, e.g.,

TEST-UNEXPECTED-FAIL | hazards | unrooted 'context' of type 'AutoTestJSContext' live across GC call at dom/indexedDB/test/gtest/TestKey.cpp:180

The AutoTestJSContext is defined as:

// TODO cf. AutoJSContext, which doesn't work here. Should it?
// This is modeled after dom/base/test/gtest/TestContentUtils.cpp
struct AutoTestJSContext {
  AutoTestJSContext()
      : mGlobalObject{mozilla::dom::SimpleGlobalObject::Create(
            mozilla::dom::SimpleGlobalObject::GlobalType::BindingDetail)} {
    EXPECT_TRUE(mJsAPI.Init(mGlobalObject));
    mContext = mJsAPI.cx();
  }

  operator JSContext*() { return mContext; }

 private:
  JSObject* mGlobalObject;
  mozilla::dom::AutoJSAPI mJsAPI;
  JSContext* mContext;
};

and instantiated on the stack in the individual gtest test cases, e.g. (this is actually the rooting hazard mentioned above):

TEST_P(TestWithParam_CString_ArrayBuffer_Pair, Ctor_EncodedBinary) {
  const auto key = Key{GetParam().first};

  ExpectKeyIsBinary(key);

  AutoTestJSContext context;

  JS::Heap<JS::Value> rv;
  EXPECT_EQ(NS_OK, key.ToJSVal(context, rv));

  CheckArrayBuffer(GetParam().second, rv);
}

I already tried adding the MOZ_STACK_CLASS annotation to AutoTestJSContext , but that does not change anything.

Any idea what I am doing wrong here?

Flags: needinfo?(allstars.chh)

Thanks for the advice! I tried adding a call to JS::AutoSuppressGCAnalysis in the AutoTestJSContext constructor and that indeed fixed the hazard-linux64-shell-haz/debug job, but not the hazard-linux64-haz/debug job.

Originally, I tried to use AutoJSContext rather than my AutoTestJSContext, but IIRC that crashed for some reason. I can check again what happens exactly if that helps.

Flags: needinfo?(allstars.chh)

replied in phab.

Flags: needinfo?(allstars.chh)
Attachment #9078757 - Attachment description: Bug 1565224 - Added some initial gtests for dom/indexedDB/Key r=ttung → Bug 1565224 - Added some initial gtests for dom/indexedDB/Key. r=ttung
Attachment #9079989 - Attachment description: Bug 1565224 - Added tests for Key::Set* and Key::CompareKeys r=ttung → Bug 1565224 - Added tests for Key::Set* and Key::CompareKeys. r=ttung
Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d2b9907f8e7
Added some initial gtests for dom/indexedDB/Key. r=ttung,asuth
https://hg.mozilla.org/integration/autoland/rev/33b6f117bd4b
Added tests for Key::Set* and Key::CompareKeys. r=ttung,asuth

This is still not resolved, as the majority of the tests is commented out because they have rooting hazards. There must be some way to resolve this.

Status: ASSIGNED → NEW
Flags: needinfo?(sgiesecke)

When removing the #if/#endif directives in https://searchfox.org/mozilla-central/rev/c80fa7258c935223fe319c5345b58eae85d4c6ae/dom/indexedDB/test/gtest/TestKey.cpp#65,75,87,97,99,138,143,161,184,208,258,428, rooting hazards are detected. Despite some hints from :allstars.chh, I was not able to resolve these yet. Steve, can you give some advice on how to fix this? If I should provide some results from a hazard analysis build, please let me know.

Flags: needinfo?(sgiesecke) → needinfo?(sphink)

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

When removing the #if/#endif directives in https://searchfox.org/mozilla-central/rev/c80fa7258c935223fe319c5345b58eae85d4c6ae/dom/indexedDB/test/gtest/TestKey.cpp#65,75,87,97,99,138,143,161,184,208,258,428, rooting hazards are detected. Despite some hints from :allstars.chh, I was not able to resolve these yet. Steve, can you give some advice on how to fix this? If I should provide some results from a hazard analysis build, please let me know.

One problem is that AutoTestJSContext has a JSObject* mGlobalObject field, which is a GC pointer, and so AutoTestJSContext is itself considered to be a GC pointer (unlike AutoJSContext, which does not have such a field.) There's a circular dependency if you wanted to root that: you want a global object so you can create an AutoJSAPI so you can get a context that is needed to root the global. The solution in TestContentUtils.cpp is to use mozilla::dom::RootingCx() for the rooting only. That should work here as well.

Further problems:

  • Do not create JS::Heap<T> objects on the stack. Those are heap-only. You want JS::Rooted<T> instead.
  • Do not pass const JS::Heap<T>& to functions. Use JS::Handle<T> instead. (And pass it by value.)
  • Don't return references to JSObjects, return pointers.
  • Definitely don't return Rooted<T>, just return T. I'm surprised that one compiled. You can lose the funky auto&& things that way too.
  • Be careful about passing JS::Value& references to functions. It will work, but only if you are very sure that the called function will not GC. Generally you want to use JS::Handle<JS::Value> instead. Though just about every JSAPI function that can GC will take a JSContext* parameter, so if you don't need one of those, you're safe.
  • Error check every single JSAPI function call.
  • For a .cpp file, consider using namespace JS if you find JS::Handle<JS::Value> too ugly. :-)
  • Style nit: outside of the toplevel js/ directory, use the templates (eg JS::Rooted<JS::Value>) instead of the typedefs (JS::RootedValue). It doesn't make a whole lot of sense for them to be different, but that's what we settled on a while back.

I'm down to one error in my patched version of this file. I'll upload a compilable version tomorrow.

Flags: needinfo?(sphink)

(In reply to Steve Fink [:sfink] [:s:] from comment #15)

Further problems:

  • Do not create JS::Heap<T> objects on the stack. Those are heap-only. You want JS::Rooted<T> instead.

Simon, I think it would be great to convert these notes to a guideline or make it a part of an existing guideline.
This stuff can be forgotten and overlooked again during reviews.

Pushed by sgiesecke@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a3de374e9241
Re-enable Key gtests and fix usage of JSAPI r=sg,dom-workers-and-storage-reviewers
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: