Intermittent e10s browser_use_counters.js | document counts for PROPERTY_FILL after are correct - Got 10, expected 9

RESOLVED FIXED in Firefox 50

Status

()

P5
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: philor, Assigned: froydnj)

Tracking

(Blocks: 1 bug, {intermittent-failure})

Trunk
mozilla52
intermittent-failure
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10s+, firefox45 wontfix, firefox47 wontfix, firefox48 wontfix, firefox49 wontfix, firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

Attachments

(2 attachments)

Blocks: 984139
tracking-e10s: --- → +
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Intermittent e10s test failure
Priority: -- → P5
Comment hidden (Intermittent Failures Robot)
status-firefox45: affected → wontfix
status-firefox47: --- → wontfix
status-firefox48: --- → wontfix
status-firefox49: --- → affected
status-firefox50: --- → affected
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
Nathan, can you please look into this frequent failure?
status-firefox49: affected → wontfix
status-firefox51: --- → affected
Flags: needinfo?(nfroyd)
Comment hidden (Intermittent Failures Robot)
(Assignee)

Comment 20

2 years ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> Nathan, can you please look into this frequent failure?

The failure looks to be specific to file_use_counter_svg_background.html.  Our test setup in that case is that we have an outer HTML document with an iframe.  The outer document loads file_use_counter_svg_background.html into the iframe, and said file has a SVG image for its background-image specified.  We then wait on the load of the iframe, assuming that after the load of the iframe completes, the SVG background image should have been decoded and appropriate use counters updated.

The only thing I can think of happening here is that the iframe has loaded (and so has the image), but we haven't painted the SVG, and therefore we haven't had to decode it and discover the appropriate CSS properties being used inside it.  Timothy, does that sound at all plausible?
Flags: needinfo?(nfroyd) → needinfo?(tnikkel)
Nathan pointed me to

https://dxr.mozilla.org/mozilla-central/rev/91c2b9d5c1354ca79e5b174591dbb03b32b15bbf/dom/svg/nsSVGElement.cpp#1218

where the use counter in question gets updated in MappedAttrParser::ParseMappedAttrValue. So sounds like it should be updated after the SVG document is parsed. And we need to parse the document before we can fire the load event on the SVG document (which in turn blocks the load event on the iframe). So I don't think painting the SVG image or not should matter.

(SVG images don't have a notion of "decoded" like raster images do, once a vector image is loaded it can be painted immediately.)

ni dholbert to confirm since he knows VectorImage better.
Flags: needinfo?(tnikkel) → needinfo?(dholbert)
(In reply to Timothy Nikkel (:tnikkel) from comment #21)
> (SVG images don't have a notion of "decoded" like raster images do, once a
> vector image is loaded it can be painted immediately.)

That's correct.  Though, it might be the case that we've received all the SVG data (so we've stopped locking onload in the host HTML document) but the parser for our internal SVG document hasn't made it through all the SVG data yet...? That's the only explanation I can think of. I think I recall the "receive data" --> "parse" pipeline being a bit asynchronous.

This might be the "rare cases" described in this comment here:
https://dxr.mozilla.org/mozilla-central/rev/91c2b9d5c1354ca79e5b174591dbb03b32b15bbf/image/VectorImage.cpp#557
>  // [...]  we send out
>  // invalidation notifications in OnSVGDocumentLoaded, but in rare cases the
>  // SVG document may not be 100% ready to render at that time.
Flags: needinfo?(dholbert)
Sorry, meant to say "stopped *blocking* onload" (not "locking")
(Assignee)

Comment 24

2 years ago
Hm, OK, thanks.  Is there something I could wait on to discover when the SVG background image has been fully loaded, then, or is that not really supported?
Flags: needinfo?(dholbert)
I don't think that's really supported right now.  We might be able to fix the "load"/onload-unblocking signalling to actually guarantee "the document's been fully parsed", though [assuming our theory is correct & it doesn't quite indicate that yet].

IIRC when I was originally writing VectorImage, that notification was sent by imgLoader or some other helper-class as soon as the last bit arrived over the network, so we didn't have the ability to wait for the parser.  But now we might be able to, with imagelib rewriting/cleanup that's happened over the years. (e.g. I'm pretty sure mProgressTracker, which mediates these notifications, was added since then)

For the time being, the best thing we can do is perhaps to poll until the count hits the right value, and if more than N time passes, call the test failed (or just let it fail by timing out)
Flags: needinfo?(dholbert)
(Assignee)

Comment 26

2 years ago
(In reply to Daniel Holbert [:dholbert] from comment #25)
> For the time being, the best thing we can do is perhaps to poll until the
> count hits the right value, and if more than N time passes, call the test
> failed (or just let it fail by timing out)

The problem here is that documents don't update the histograms associated with their use counters eagerly; the use counters themselves are noted eagerly, but the histograms associated with the use counters aren't updated until document destruction.  We eagerly update the histograms in the test code with nsIDOMWindowUtils:

http://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#1930

but there's no way that I know of to get a handle on the SVG image to have it update its parent document's use counters.  I'm not sure how to poll, either; we can't poll on the histogram itself, since the histogram isn't actually getting updated, and we can't repeatedly call through that nsIDOMWindowUtils method, since the underlying use counter code only permits the histograms to be updated once (to avoid multiple counting of things).

Maybe robustifying the load/onload-unblocking to guarantee the SVG document has been fully parsed would be the way to go, but I wonder if that would be web-compatible...
Hmm, maybe a small timeout after the document load would be sufficient then?  (Yes, timeouts are evil.)

Robustifying the load/onload-unblocking would probably be web-compatible -- but it would also make us strictly slower at loading pages, and it shouldn't be a necessary/helpful change for standard use-cases -- so it might not really be a change we actually want to make.

If we really need it, we could probably add a new image status bit, which would only be visible to code with chrome privileges -- something like the bits we check here:
 https://dxr.mozilla.org/mozilla-central/rev/91c2b9d5c1354ca79e5b174591dbb03b32b15bbf/image/test/mochitest/imgutils.js#26
...and the test could request special powers and then poll until that status is set, for example.
(Assignee)

Comment 28

2 years ago
(In reply to Daniel Holbert [:dholbert] from comment #27)
> If we really need it, we could probably add a new image status bit, which
> would only be visible to code with chrome privileges -- something like the
> bits we check here:
>  https://dxr.mozilla.org/mozilla-central/rev/
> 91c2b9d5c1354ca79e5b174591dbb03b32b15bbf/image/test/mochitest/imgutils.js#26
> ...and the test could request special powers and then poll until that status
> is set, for example.

That depends on actually having a handle to an <img> node or similar, right?  We don't have that for background-images of elements, do we?  (The document itself can't be an nsIImageLoadingContent for the background image...?)
Flags: needinfo?(dholbert)
Oh, hmm, I think you're right. That raises two questions in my mind:
 (1) Do we really need to test this for *specifically background-images*? I'd think we could just test this for <img> and then assume it'll work just as well for background/border/list-style-image, as long as there's not some <img>-element-specific aspect to the counter codepath here.

Put another way: Could we just rewrite the test to use <img> (so that we could add an internal-only image load flag), and/or can we lean on existing (presumably passing) <img> tests?

 (2) Do our existing <img> tests trigger this same bug? (If not, that's a little interesting.)
Flags: needinfo?(dholbert)
(Assignee)

Comment 30

2 years ago
(In reply to Daniel Holbert [:dholbert] from comment #29)
> Oh, hmm, I think you're right. That raises two questions in my mind:
>  (1) Do we really need to test this for *specifically background-images*?
> I'd think we could just test this for <img> and then assume it'll work just
> as well for background/border/list-style-image, as long as there's not some
> <img>-element-specific aspect to the counter codepath here.
> 
> Put another way: Could we just rewrite the test to use <img> (so that we
> could add an internal-only image load flag), and/or can we lean on existing
> (presumably passing) <img> tests?

I was attempting to be complete by testing SVGs-loaded-from-CSS in addition to <img>, etc., but if you think that's overkill and existing <img> and similar tests cover the same ground, I can happily delete these tests.

>  (2) Do our existing <img> tests trigger this same bug? (If not, that's a
> little interesting.)

AFAICT from orangefactor, they do not.  I would expect intermittents to show up for these tests:

http://dxr.mozilla.org/mozilla-central/source/dom/base/test/browser_use_counters.js#37

and we don't seem to have any.
Flags: needinfo?(dholbert)
(In reply to Nathan Froyd [:froydnj] from comment #30)
> I was attempting to be complete by testing SVGs-loaded-from-CSS in addition
> to <img>, etc., but if you think that's overkill and existing <img> and
> similar tests cover the same ground, I can happily delete these tests.

Yeah, I think that'd be fine.  We already have tests that ensure that SVG works (renders, animates, etc) as a background & as a <img>, and those two use-cases use the exact same imagelib internals for creating/managing the internal SVG document.

So, I think it's fine to just test this telemetry for whichever SVG-as-an-image use-case is most convenient, since the code we're testing happens at a level that's several layers of abstraction lower than any particular way of referencing the image (and hence should be independent of how the image is referenced).  And if SVG-as-an-image backgrounds still paint (tested via our other tests), we can assume that they've gotten far enough to trigger this telemetry code (tested via these <img> tests you mentioned).

> >  (2) Do our existing <img> tests trigger this same bug? (If not, that's a
> > little interesting.)
> 
> AFAICT from orangefactor, they do not.

Huh! I do have some memory that maybe onload-blocking is supposed to work differently for backgrounds vs. content like <img>, so that might explain this. I'm not sure though. Anyway, that's good news from a we-can-just-lean-on-those-tests-a-bit-more perspective. :)
Flags: needinfo?(dholbert)
Comment hidden (Intermittent Failures Robot)
Comment hidden (Intermittent Failures Robot)
(Assignee)

Comment 34

2 years ago
Created attachment 8794316 [details] [diff] [review]
part 1 - remove unused xlink:href attributes

Nothing in the tests uses them, and having them makes the tests spew
unnecessary warnings.  Let's remove them.

Selecting dholbert to review these: even though your expertise is not DOM,
you've been commenting here and I figure you have as much context as anybody
for these patches.  Please feel free to reassign the review!
Attachment #8794316 - Flags: review?(dholbert)
(Assignee)

Comment 35

2 years ago
Created attachment 8794319 [details] [diff] [review]
part 2 - remove SVG background image tests from browser_use_counters.js

These tests are racy, as we can receive all the data for the SVG image,
thereby unblocking the onload event, but the actual SVG image may not be
fully decoded, which means that we're not getting the use counters
updated that we need for the test.  Other SVG image tests should go
through the same codepaths as rendering SVG images for backgrounds,
though, so these tests are superfluous.

It's a little hard to tell because of mozharness failures, but:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c7022792c85

shows no instances of this intermittent...
Attachment #8794319 - Flags: review?(dholbert)
Comment hidden (Intermittent Failures Robot)
Comment on attachment 8794316 [details] [diff] [review]
part 1 - remove unused xlink:href attributes

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

This seems fine; r=me.
Attachment #8794316 - Flags: review?(dholbert) → review+
Comment on attachment 8794319 [details] [diff] [review]
part 2 - remove SVG background image tests from browser_use_counters.js

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

r=me, with one nit:

::: dom/base/test/browser_use_counters.js
@@ +67,2 @@
>    yield check_use_counter_iframe("file_use_counter_svg_list_style_image.html",
>                                   "PROPERTY_FILL");

(observation): Hmm, I'm a bit surprised that we don't have the same issue for "list-style-image" that we're hitting for "background".  Those two CSS features should take approximately the same codepath, as far as load blocking etc., I'd expect. *shrug*

@@ +75,5 @@
> +                                 "SVGSVGELEMENT_GETELEMENTBYID", false);
> +  yield check_use_counter_iframe("file_use_counter_svg_currentScale.svg",
> +                                 "SVGSVGELEMENT_CURRENTSCALE_getter", false);
> +  yield check_use_counter_iframe("file_use_counter_svg_currentScale.svg",
> +                                 "SVGSVGELEMENT_CURRENTSCALE_setter", false);

It looks like this section (about loads from the cache) is meant to *immediately follow* the tests that put the images in the cache.  

(That's how it was, before this patch, when we were referencing the background-image test file here. But now we're referencing some test files that we loaded way further up.)

So: if possible, please move this section up to happen just after the tests that put these images in the cache. (That way, the function calls are alongside their analogous cache-populating calls, and it's easier to see what we're doing.
Attachment #8794319 - Flags: review?(dholbert) → review+

Comment 39

2 years ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19a1bd113a83
part 1 - remove unused xlink:href attributes; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe97a239ab5a
part 2 - remove SVG background image tests from browser_use_counters.js; r=dholbert

Comment 40

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/19a1bd113a83
https://hg.mozilla.org/mozilla-central/rev/fe97a239ab5a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee: nobody → nfroyd
You need to log in before you can comment on or make changes to this bug.