Closed Bug 1224873 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Core & HTML, defect, P5)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
e10s + ---
firefox45 --- wontfix
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: philor, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

part 1 - remove unused xlink:href attributes
2.19 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
part 2 - remove SVG background image tests from browser_use_counters.js
3.05 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
Blocks: e10s-tests
tracking-e10s: --- → +
Intermittent e10s test failure
Priority: -- → P5
Nathan, can you please look into this frequent failure?
status-firefox49: affected → wontfix
status-firefox51: --- → affected
Flags: needinfo?(nfroyd)
(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")
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)
(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.
(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)
(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)
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)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/19a1bd113a83
https://hg.mozilla.org/mozilla-central/rev/fe97a239ab5a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee: nobody → nfroyd
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: