Bug 228569 – storage/indexeddb/detached-iframe.html is flaky on some bots

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Dumez]

storage/indexeddb/detached-iframe.html is flaky on some bots:

TEXT DIFF:
 ALERT: original value
+indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
+
+indexedDB.deleteDatabase(dbname)
+indexedDB.open(dbname)
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Chris Dumez]

<rdar://80396559>

[Chris Dumez]

Created attachment 434472 [details]
Patch

[Sihui Liu]

Comment on attachment 434472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434472&action=review

> LayoutTests/storage/indexeddb/detached-iframe.html:7
> +jsTestIsAsync = true;

Isn't it set in shared.js?

[Chris Dumez]

Comment on attachment 434472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434472&action=review

>> LayoutTests/storage/indexeddb/detached-iframe.html:7
>> +jsTestIsAsync = true;
> 
> Isn't it set in shared.js?

Hmm, it does. Yet, my change is causing the output to show up so there is a behavior change somehow..

[Chris Dumez]

Comment on attachment 434472 [details]
Patch

Will investigate further.

[Sihui Liu]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 434472 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434472&action=review
> 
> >> LayoutTests/storage/indexeddb/detached-iframe.html:7
> >> +jsTestIsAsync = true;
> > 
> > Isn't it set in shared.js?
> 
> Hmm, it does. Yet, my change is causing the output to show up so there is a
> behavior change somehow..

Probably output from description()

[Chris Dumez]

Comment on attachment 434472 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434472&action=review

> LayoutTests/storage/indexeddb/detached-iframe-expected.txt:2
> +IndexedDB edge case with a detached iframe.

description() would add this logging ...

> LayoutTests/storage/indexeddb/detached-iframe-expected.txt:4
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

... and this one.

> LayoutTests/storage/indexeddb/detached-iframe-expected.txt:7
> +indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;

description() does not explain this logging?

[Sihui Liu]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 434472 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434472&action=review
> 
> > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:2
> > +IndexedDB edge case with a detached iframe.
> 
> description() would add this logging ...
> 
> > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:4
> > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> ... and this one.
> 
> > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:7
> > +indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
> 
> description() does not explain this logging?

Right, I thought these lines should always be printed. Something wrong with the DOM nodes...

testIframe.parentNode.remove(testIframe);
should probably be:
testIframe.parentNode.removeChild(testIframe);

[Chris Dumez]

(In reply to Sihui Liu from comment #8)
> (In reply to Chris Dumez from comment #7)
> > Comment on attachment 434472 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=434472&action=review
> > 
> > > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:2
> > > +IndexedDB edge case with a detached iframe.
> > 
> > description() would add this logging ...
> > 
> > > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:4
> > > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > 
> > ... and this one.
> > 
> > > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:7
> > > +indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
> > 
> > description() does not explain this logging?
> 
> Right, I thought these lines should always be printed. Something wrong with
> the DOM nodes...
> 
> testIframe.parentNode.remove(testIframe);
> should probably be:
> testIframe.parentNode.removeChild(testIframe);

It's equivalent, so is `testIframe.remove()`. It has no impact on the output of the test. Just adding the description() makes the output show (seemingly consistently) but I don't have an explanation why at the moment...

[Chris Dumez]

Created attachment 434523 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #9)
> (In reply to Sihui Liu from comment #8)
> > (In reply to Chris Dumez from comment #7)
> > > Comment on attachment 434472 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=434472&action=review
> > > 
> > > > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:2
> > > > +IndexedDB edge case with a detached iframe.
> > > 
> > > description() would add this logging ...
> > > 
> > > > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:4
> > > > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > > 
> > > ... and this one.
> > > 
> > > > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:7
> > > > +indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
> > > 
> > > description() does not explain this logging?
> > 
> > Right, I thought these lines should always be printed. Something wrong with
> > the DOM nodes...
> > 
> > testIframe.parentNode.remove(testIframe);
> > should probably be:
> > testIframe.parentNode.removeChild(testIframe);
> 
> It's equivalent, so is `testIframe.remove()`. It has no impact on the output
> of the test. Just adding the description() makes the output show (seemingly
> consistently) but I don't have an explanation why at the moment...

I checked with MutationObserver, testIframe.parentNode.remove(testIframe) removes document.body, testIframe.parentNode.removeChild(testIframe) removes iframe. Removing document.body makes the console element added in different places (function getOrCreate(id, tagName)@js-tests.js)...

[Chris Dumez]

(In reply to Sihui Liu from comment #11)
> (In reply to Chris Dumez from comment #9)
> > (In reply to Sihui Liu from comment #8)
> > > (In reply to Chris Dumez from comment #7)
> > > > Comment on attachment 434472 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=434472&action=review
> > > > 
> > > > > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:2
> > > > > +IndexedDB edge case with a detached iframe.
> > > > 
> > > > description() would add this logging ...
> > > > 
> > > > > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:4
> > > > > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> > > > 
> > > > ... and this one.
> > > > 
> > > > > LayoutTests/storage/indexeddb/detached-iframe-expected.txt:7
> > > > > +indexedDB = self.indexedDB || self.webkitIndexedDB || self.mozIndexedDB || self.msIndexedDB || self.OIndexedDB;
> > > > 
> > > > description() does not explain this logging?
> > > 
> > > Right, I thought these lines should always be printed. Something wrong with
> > > the DOM nodes...
> > > 
> > > testIframe.parentNode.remove(testIframe);
> > > should probably be:
> > > testIframe.parentNode.removeChild(testIframe);
> > 
> > It's equivalent, so is `testIframe.remove()`. It has no impact on the output
> > of the test. Just adding the description() makes the output show (seemingly
> > consistently) but I don't have an explanation why at the moment...
> 
> I checked with MutationObserver, testIframe.parentNode.remove(testIframe)
> removes document.body, testIframe.parentNode.removeChild(testIframe) removes
> iframe. Removing document.body makes the console element added in different
> places (function getOrCreate(id, tagName)@js-tests.js)...

Oh, you are right that testIframe.parentNode.remove(testIframe) seems very wrong. It removes the parent node instead of the frame, LOL :)
I got confused because ParentNode.idl has append/prepend (but not remove).

I'll test again but I believe fixing that didn't make the output appear though. For me, it was switching to js-test-pre.js/js-test-post.js (or calling description()) that seemed to do the trick.

[Chris Dumez]

Comment on attachment 434523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434523&action=review

> LayoutTests/storage/indexeddb/detached-iframe.html:8
> +    document.getElementById('testIframe').remove();

Actually, this fix alone is sufficient to make the output show. I got confused because the test is marked as flaky internally.

I'll revert the js-test-pre.js / js-test-post.js change.

[Chris Dumez]

Created attachment 434527 [details]
Patch

[EWS]

Committed r280426 (240066@main): <https://commits.webkit.org/240066@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 434527 [details].

[Chris Dumez]

*** Bug 207844 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Unskipped test in <https://commits.webkit.org/r280427>.