704059 - Decouple OnStopRequest and OnStopDecode for VectorImage

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

(see bug 656244 comment 30 for a little backstory on this)

So -- imgRequest::OnStopRequest() fires when we're finished receiving data, and it tells image consumers that we're finished loading/ready to paint. (I think via imgStatusTracker::RecordStopRequest)

However, for SVG images, these events (finished receiving data & finished loading) aren't intrinsically linked.  We may need more time to finish parsing, constructing the DOM, doing layout, loading internal images, etc.

We hackily handle this right now by flushing parsing & layout inside of SVGDocumentWrapper::OnStopRequest, but that's undesirable and insufficient.  It's undesirable because a specially-crafted SVG image could easily make itself take a long time to layout, and that would effectively DOS us inside of those flushes (using just an image).  And it's insufficient because images *inside* our SVG image may or may not be loaded at that point, and I'm not sure we can synchronously ensure that they're loaded.  (see bug 703806 comment 1 for an example of that).

So -- we really need to decouple "finished receiving data" from "Image loaded" inside of ImageLib.

Comment 1

[Daniel Holbert [:dholbert]]

(and once we've done this decoupling, we'd want to wait for "SVGLoad" to be fired in the internal SVG document before sending the "image loaded" message to consumers.)

Comment 2

[Bobby Holley (:bholley)]

While we're at it, we should rethink all of these observer notifications in a way that makes sense. For example, right now OnStopDecode and OnStopRequest always have to be fired at the same time, because OnStopDecode is the method that includes the network load status. So we fire OnStopContainer for decode completion, which is a gross hack.

All of this is discussed in detail over in bug 505385.

Comment 4

[Seth Fowler [:seth] [:s2h]]

These days (post bug 505385) we can express this distinction pretty well using OnStopRequest to indicate "finished receiving data" and OnStopDecode to indicate "the image is actually displayable". RasterImage/Decoder already do this (as usual that's the code path that's received the most love) but right now VectorImage does not take advantage of this separation. This is causing intermittent oranges, at least bug 772443 and almost certainly others. So what we need to do is perform this decoupling for VectorImage and not fire OnStopDecode until SVGLoad gets fired.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: Decouple OnStopRequest and OnStopDecode for VectorImage.

Proposed patch. This should clear bug 772443 right up. Running a try job here: https://tbpl.mozilla.org/?tree=Try&rev=d49c22732b06

Comment 6

[Seth Fowler [:seth] [:s2h]]

To clarify the cause of the oranges, when SVG images fired OnStopDecode immediately after OnStopRequest, sometimes the SVG document wasn't yet fully constructed. This would cause the document's onload event to fire before we could actually render the SVG image, and depending on how the race turned out nothing might be displayed onscreen.

Comment 7

[Seth Fowler [:seth] [:s2h]]

Ack, there's an issue with error handling that broke some cases on try. I've found the problem; will upload a modified patch shortly.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: Decouple OnStopRequest and OnStopDecode for VectorImage.

OK, fixed the error handling issue in the previous patch. Now even if the SVG is invalid the right notifications get delivered. Everything should be good to go now. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=3170ea252f68

Comment 9

[Seth Fowler [:seth] [:s2h]]

Attachment: Decouple OnStopRequest and OnStopDecode for VectorImage.

Noticed a couple of additional issues. This version should free memory faster and uses a better test for whether the SVG document has already finished loading. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=d5ea64abcbb0

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 702155 [details] [diff] [review]
Decouple OnStopRequest and OnStopDecode for VectorImage.

Hmm, I won't bother with the feedback flag, but let me ask: is there no better way of seeing with an SVG document has already fired its SVGLoad event than this?

> nsIDocument* mySVGDocument = GetDocument();
> if (mySVGDocument->GetRootElement() && mySVGDocument->GetRootElement()->GetPrimaryFrame()) {
>   // SVGLoad has already fired.
> }

This isn't exactly how it looks in the patch - it's hidden behind VectorImage::OnStopRequest calling mSVGDocumentWrapper->GetRootLayoutFrame() - but that's what the test boils down to, and I just want to make sure that this seems reasonable. In informal testing, when this test isn't yet true in OnStopRequest and I create a DOM event listener for SVGLoad, the test becomes true by the time SVGLoad fires. Thus it looks like it works but this feels a bit brittle and I'm wondering if there's a better option. I looked at nsIDocument::GetDocumentState but that didn't seem to do what I needed.

Comment 11

[Seth Fowler [:seth] [:s2h]]

s/with/whether

Comment 12

[Joe Drew (not getting mail)]

Comment on attachment 702155 [details] [diff] [review]
Decouple OnStopRequest and OnStopDecode for VectorImage.

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

r=me with some nits, but Daniel will need to look at this too, as I'm not qualified to review the SVG-specific bits.

::: image/src/VectorImage.cpp
@@ +127,5 @@
> +private:
> +  void CleanUp()
> +  {
> +    if (mDocument) {
> +      mDocument->RemoveEventListener(NS_LITERAL_STRING("SVGLoad"), this, true);

The last argument of this function needs, AIUI, to be the same as the last argument of AddEventListener, since it specifies whether the event listener was registered as a capturing event listener, and doesn't remove the listener if it doesn't match.

@@ +753,5 @@
> +
> +  // If the SVG document's not ready, create an observer to wait for it to finish.
> +  if (!mSVGDocumentWrapper->GetRootLayoutFrame()) {
> +    mDOMEventObserver = new SVGDOMEventObserver(document, this);
> +    return mSVGDocumentWrapper->OnStopRequest(aRequest, aCtxt, aStatus);

I'd prefer it if this was done unconditionally, and OnSVGDocumentLoaded was the part that was documented as firing events.

@@ +780,5 @@
>      mError = true;
> +    if (observer)
> +      observer->OnStopDecode(NS_ERROR_FAILURE);
> +
> +    return;

Can you refactor this function so it doesn't need the bare return, making the control flow more obvious?

Comment 13

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Joe. I'll make those changes.

> I'd prefer it if this was done unconditionally, and OnSVGDocumentLoaded was the part that was documented as firing events.

So would I. =) That's how things were structured originally, but the problem is that for very tiny SVGs we may miss the SVGLoad event. If you register the DOM event listener too late, you're out of luck, and we can sit there spinning without completing the page load forever. That's really what I'm trying to avoid by not creating the observer if the document is ready. (Probably this should be more clearly explained in the comment; I'll add that.)

If Boris or Daniel can suggest a better approach here, I'd love that; as I mentioned in my comment above, I don't think this is as clean as it should be.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 702155 [details] [diff] [review]
Decouple OnStopRequest and OnStopDecode for VectorImage.

>--- a/image/src/VectorImage.cpp
>+++ b/image/src/VectorImage.cpp
>@@ -1,9 +1,9 @@
>-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+///* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

  ^^ This looks like an accidental addition. Remove?

>+class SVGDOMEventObserver : public nsIDOMEventListener {

Nit: That name is a little generic, for a class that literally just cares about one event.

Maybe call it "SVGLoadEventListener", since that's the only event we actually care to listen for?  Similarly, mDOMEventObserver should probably be renamed to mSVGLoadListener.

>+public:
>+  NS_DECL_ISUPPORTS
>+
>+  SVGDOMEventObserver(nsIDocument* aDocument,
>+                      VectorImage* aImage)
>+    : mDocument(aDocument)
>+    , mImage(aImage)
>+  {
>+    if (mDocument)
>+      mDocument->AddEventListener(NS_LITERAL_STRING("SVGLoad"), this, true, false);
>+  }

I don't think you need to null-check mDocument -- this is only created in one spot, and in that spot we've already null-checked the document that's passed in.

Probably worth asserting, at the beginning of the constructor, that both mDocument & aImage are both non-null, though.

>+  NS_IMETHOD HandleEvent(nsIDOMEvent* /* aEvent */) MOZ_OVERRIDE
>+  {

I think the pattern I've seen elsewhere is to have HandleEvent() nominally check |aEvent| (perhaps with an assertion) to be sure it's the event you're expecting.  Not strictly necessary, but nice as a sanity-check.

Also: HandleEvent should probably assert that mDocument is non-null.  Pretty sure that's guaranteed (or should be) right now.
 
>@@ -691,44 +736,82 @@ VectorImage::OnStopRequest(nsIRequest* a
>+  nsIDocument* document = mSVGDocumentWrapper->GetDocument();
>+  if (!document)
>+    return NS_ERROR_FAILURE;

What does it mean if |document| is null here?  Might be good to add a comment explaining what that means. Also, possibly worth setting |mError = true|? (It doesn't look like there's any path forward to a properly-rendered image, if we take this early-return...)

>+  // If the SVG document's not ready, create an observer to wait for it to finish.
s/ready/finished loading/

>+  if (!mSVGDocumentWrapper->GetRootLayoutFrame()) {

I'm not sure that's the right thing to check. If the SVG document has e.g. an internal <image> (which maybe points to blob URI for another SVG document, which _itself_ isn't finished loading), then I think it could have a root nsIFrame but not have fired its SVGLoad event yet.

>+    mDOMEventObserver = new SVGDOMEventObserver(document, this);
>+    return mSVGDocumentWrapper->OnStopRequest(aRequest, aCtxt, aStatus);
>+  }
>+
>+  // There's no need to wait, so go ahead and fire events. Note that the
>+  // ordering here is the same as in the other case, because SVGDOMEventObserver
>+  // would have called OnSVGDocumentLoaded later, asynchronously.
>   nsresult rv = mSVGDocumentWrapper->OnStopRequest(aRequest, aCtxt, aStatus);

This can likely be cleaner (as you & joe discussed above).

How about: mDOMEventObserver (a.k.a. mSVGLoadListener) checks for "Has SVGLoad already been fired?" in its constructor (the equivalent of your GetRootLayoutFrame check) -- and if it has, then it your event-listener object immediately calls OnSVGDocumentLoaded and even never bothers to register to listen for events.

(As I noted above, I'm not sure offhand what the best way to check "Has SVGLoad already been fired?" is, offhand.  Your current method -- checking whether we've got a root frame -- might be OK as a first approximation (e.g. check  document->GetRootElement() && document->GetRootElement()->GetPrimaryFrame()), at least for testing purposes...  Though ideally I'd like to figure out something more foolproof that handles the internal <image> case that I mentioned above.

>diff --git a/image/src/VectorImage.h b/image/src/VectorImage.h
>   bool           mIsFullyLoaded:1;        // Has OnStopRequest been called?

This bool needs a documentation update. Maybe "// Has internal doc fired SVGLoad?"

Comment 15

[Boris Zbarsky [:bzbarsky]]

When does the relevant SVGLoad fire?  Is it more like DOMContentLoaded, or more like onload, or something else

And most importantly, can the readyState of the document tell you anything about whether SVGLoad has fired?

Comment 16

[Daniel Holbert [:dholbert]]

For a standalone SVG document, I believe it's the same as "onload".
Spec reference: http://www.w3.org/TR/SVG11/script.html#LoadEvent

MDN's overview indicates that SVGLoad requires that external resources have already been loaded & are ready to display. 
 https://developer.mozilla.org/en-US/docs/Mozilla_event_reference/SVGLoad

Comment 17

[Daniel Holbert [:dholbert]]

...though in practice, it looks like we dispatch SVGLoad as soon as we parse "</svg>"...
https://mxr.mozilla.org/mozilla-central/source/content/xml/document/src/nsXMLContentSink.cpp#1132  So maybe that makes it (for us) closer to DOMContentLoaded.

Comment 18

[Daniel Holbert [:dholbert]]

I think we might want to start listening for SVGLoad in OnStartRequest, near where we call   mViewer->GetDocument()->SetIsBeingUsedAsImage().  That's before the parser has gotten a chance to start munging the XML, and it's guaranteed to be before SVGLoad fires.

Comment 19

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #14)

Thanks for the review, Daniel! I'll make the changes you requested. Some responses below.

> What does it mean if |document| is null here?

I think we'd only see this if notifications got delivered in an unexpected order or if SVGDocumentWrapper got destroyed at an unexpected time. Probably better off as an assert; I did it this way to avoid crashes. Is there a macro that handles that nicely or should I just use NS_ABORT_IF_FALSE and not worry about the possibility of crashing on release builds?

> This can likely be cleaner (as you & joe discussed above).
> 
> How about: mDOMEventObserver (a.k.a. mSVGLoadListener) checks for "Has
> SVGLoad already been fired?" in its constructor (the equivalent of your
> GetRootLayoutFrame check) -- and if it has, then it your event-listener
> object immediately calls OnSVGDocumentLoaded and even never bothers to
> register to listen for events.

I like the idea of moving these different code paths into (the class currently known as) SVGDOMEventObserver, but I don't like the idea of adding even more side effects to the constructor. ANY side effects in the constructor is unclean to me. I'll make a separate method that actually does the event handler registration or OnSVGDocumentLoaded call. This also has the advantage that it means we don't try to delete the object from inside a call to its constructor. =)

> (As I noted above, I'm not sure offhand what the best way to check "Has
> SVGLoad already been fired?" is, offhand.  Your current method -- checking
> whether we've got a root frame -- might be OK as a first approximation (e.g.
> check  document->GetRootElement() &&
> document->GetRootElement()->GetPrimaryFrame()), at least for testing
> purposes...  Though ideally I'd like to figure out something more foolproof
> that handles the internal <image> case that I mentioned above.

Both David Baron and Boris (thanks!) suggested GetReadyState; I'll give that a shot and report back. I've now confirmed that the current approach, in addition to being brittle and inelegant, does not actually work; it causes problems on try, although (as usual, alas) it works fine locally.

Comment 20

[Seth Fowler [:seth] [:s2h]]

@Daniel: Yeah, I had considered that approach last night, but there was something about the interaction pattern between the various objects that made it awkward. (Though doable, for sure.) Let me try again now that I've gotten some sleep.

Comment 21

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> ...though in practice, it looks like we dispatch SVGLoad as soon as we parse
> "</svg>"...

Hmm, does that mean it's actually the wrong thing to use? Hopefully the document is renderable at that point, even if external images and the like haven't loaded.

Comment 22

[Daniel Holbert [:dholbert]]

I think it's still the right thing to use, though using "load" should be fine too.  (If it turns out that some external resources aren't loaded when SVGLoad fires, we should probably file separate bugs on that & fix that.)

Anyway -- I'm guessing GetReadyState() corresponds to when "load" fires, so if ends up being more consistent to just deal with "load", that sounds good to me.

Comment 23

[Seth Fowler [:seth] [:s2h]]

I've tracked down the persistent try failures related to zero-size-image.svg. This (edited) IRC transcript explains my findings and the plan Daniel and I discussed to solve the problem:

[5:08pm] seth: dholbert: i have continued down the rabbit hole of this SVG OnStopRequest bug and i have discovered, i believe, an even more insidious bug
[5:08pm] seth: dholbert: basically, content/svg/content/src/crashtests/zero-size-image.svg is an svg that _contains_ another svg as an image
[5:08pm] seth: dholbert: that inner svg is just a data URI that contains "<svg></svg>"
[5:08pm] seth: dholbert: so we never get SVGLoad for that inner SVG, which ends up blocking onload for the outer document with the new patch
[5:09pm] seth: dholbert: why? because you recall how we dispatch SVGLoad in XMLContentSink? we decide whether to do that by checking whether the top-level element is an "svg" element
[5:09pm] seth: dholbert: BUT, we also check its namespace, and that data URI svg doesn't have the proper namespace
[5:09pm] seth: dholbert: result: we never dispatch SVGload
[5:10pm] dholbert: seth, hmm.... yeah, so what you're saying is: if an SVG document is broken, it won't fire SVGLoad... and that means if we use SVGLoad to indicate "image finished loading", then we won't ever fire that
[5:13pm] dholbert: seth, so I think this means that we can'
[5:13pm] dholbert: seth, *we can't rely on SVGLoad being fired 
[5:13pm] seth: dholbert: yeah. 
[5:13pm] seth: dholbert: onload should get fired no matter what, though, right?
[5:13pm] seth: dholbert: maybe that's the quickest fix here
[5:13pm] dholbert: seth, I think so, yeah
[5:14pm] dholbert: seth, yeah, that makes sense to me
[5:15pm] dholbert: seth, it might even be "most correct" to listen both for "load" _and_ "SVGLoad", and accept whichever one you get first
[5:18pm] seth: dholbert: sounds good. i'll add a comment about this situation as it is decidedly nonobvious

Comment 24

[Seth Fowler [:seth] [:s2h]]

A further update: we can't listen for the SVG document's onload event because the onload event doesn't get sent unless the document is attached to a window. It turns out that my very first approach to handling this, which is to use nsIDocumentObserver::EndLoad, is probably the best way. Unfortunately I foolishly didn't document why I turned away from that approach on the bug and now I've lost track. It may be that whatever it was was fixed by some of the other changes that have been made since then, so I'll switch back to nsIDocumentObserver::EndLoad and hopefully everything will work just fine.

Comment 25

[Seth Fowler [:seth] [:s2h]]

Attachment: Decouple OnStopRequest and OnStopDecode for VectorImage.

It turns out that EndLoad won't work because it fires too early, before the document is ready to be rendered, and thus reintroduces the race. After some further experimentation and discussion with Daniel I've settled on sticking with SVGLoad, but cancelling the listener in OnStopRequest if I detect that something seems wrong with the SVG document. In local tests this seems to work perfectly. I've started a try run here: https://tbpl.mozilla.org/?tree=Try&rev=9c2181c66ffb

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to Seth Fowler [:seth] from comment #25)
> but cancelling the listener in OnStopRequest if I detect that
> something seems wrong with the SVG document.

Since we're no longer flushing the parser, I don't think we're _guaranteed_ that we'd have any of the document (including the root node) parsed yet, in OnStopRequest. :(

In practice, it's likely we will... but consider e.g. an example with a giant <!--... --> or a particularly long <?xml ...> declaration, before the opening <svg> node (and then very little inside the <svg>) -- in that case, we could get to OnStopRequest while the parser still hasn't made it through the pre-<svg> stuff, I'm pretty sure.

Comment 27

[Daniel Holbert [:dholbert]]

(sorry for not pointing that out in IRC.  I misread your question -- you said OnStopRequest, and I read it as OnLoad. :-/)

Comment 28

[Seth Fowler [:seth] [:s2h]]

Ah, drat, good point. That's how I had it originally, but I foolishly deleted that code when I impemented the SVGError approach we talked about and didn't restore it when I switched to this method. I'll restore the wait for EndLoad.

Comment 29

[Daniel Holbert [:dholbert]]

That sounds good. (I don't know the specifics of EndLoad, but it sounds like the right sort of thing to listen for.)

Comment 30

[Seth Fowler [:seth] [:s2h]]

Attachment: Decouple OnStopRequest and OnStopDecode for VectorImage.

Restored the wait for EndLoad. New try job here: https://tbpl.mozilla.org/?tree=Try&rev=c470ecab92c6

Comment 31

[Daniel Holbert [:dholbert]]

That try run is looking green (woot!), except for one WinXP reftest orange due to an assertion-failure.

The assertion-failure makes sense -- previously, mIsFullyLoaded (which guards access to the internal SVG document) and mHaveAnimations couldn't be set before OnStopRequest, but your patch in that Try push now allows them to be set earlier, in OnSVGDocumentLoaded.

So -- you probably want to just move the assertion about those flags to the beginning of OnSVGDocumentLoaded.


(One other nit: in your added assertion messages, drop the period at the end -- assert messages are displayed with a ":" after them, so if they end with a period, they end up with ".:" as shown in e.g. bug 526536 comment 0, which looks silly)

Comment 32

[Seth Fowler [:seth] [:s2h]]

Thanks for looking over the try results, Daniel. I'll update the patch and run it through try again later today.

Comment 33

[Seth Fowler [:seth] [:s2h]]

Attachment: Decouple OnStopRequest and OnStopDecode for VectorImage.

Moved misplaced assertions, removed superfluous periods from assertion messages, and got rid of leftover debugging code.

Comment 34

[Seth Fowler [:seth] [:s2h]]

Got distracted from this bug for a bit, but I'm hoping we can get this done today. Tired of bugspam from bug 772443. =)

I started a try job here: https://tbpl.mozilla.org/?tree=Try&rev=f41c2186cafe

Comment 35

[Daniel Holbert [:dholbert]]

Comment on attachment 707351 [details] [diff] [review]
Decouple OnStopRequest and OnStopDecode for VectorImage.

Overall, looks great! Just a few nits on the final patch (followup up on my prior review in comment 14):

>+class SVGParseCompleteListener : public nsStubDocumentObserver {
[...]
>+  SVGParseCompleteListener(nsIDocument* aDocument,
>+                           VectorImage* aImage)
>+    : mDocument(aDocument)
>+    , mImage(aImage)
>+  { }

This constructor probably wants to assert that mDocument and mImage are both non-null.  (Your assertions in Dispatch kind of cover this, but might as well assert it when we initialize those variables, and there's no harm in asserting it in both places.) (but see also my lower review-comment about maybe merging Dispatch into the constructor, which sort of overrides this comment.)

>+  virtual ~SVGParseCompleteListener() { 
>+    Cancel();
>+  }

Nit: bump the open-brace down, to be consistent with the other nearby code.

Also: I'm almost certain this destructor doesn't want to be declared as virtual. The class's virtual, macro-provided Release() method is what *actually* ends up deleting us, and it knows our exact derived type, so a normal destructor should be fine.  (And if I happen to be wrong about that for some reason, then we're still fine w/o the virtual keyword in *this* spot -- as long as *we* don't have any subclasses, then the place where 'virtual' would matter is on the ancestor's destructor, not on our destructor.)

So: drop the "virtual", and declare the class as MOZ_FINAL to be sure nothing inherits from us.

>+  void Dispatch()
>+  {
>+    NS_ABORT_IF_FALSE(mDocument, "Need an SVG document");
>+    NS_ABORT_IF_FALSE(mImage, "Need an image");
>+
>+    mDocument->AddObserver(this);
>+  }

Can't we just merge this into the constructor? I don't see why we need a separate method for this, and it'd make it clearer that this code runs exactly once, and it's to be run when the class is constructed.

>+  void Cancel()
>+  {
>+    if (mDocument) {
>+      mDocument->RemoveObserver(this);
>+      mDocument = nullptr;
>+    }
>+  }

That "if (mDocument)" check is only necessary when Cancel gets called by the destructor -- it's unnecessary when we're called by EndLoad.

So: I'd suggest pulling that null-check out, and putting it in the destructor, and make Cancel() assume (maybe assert?) that mDocument is non-null.

>+class SVGLoadEventListener : public nsIDOMEventListener {

The above review-comments all apply to this helper-class as well (specifically, the note about 'virtual', about Dispatch()-maybe-merging-into-constructor, and about pulling out Cancel()'s null-check).

>+  NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent) MOZ_OVERRIDE
>+  {
[...]
>+    NS_ABORT_IF_FALSE(eventType.EqualsLiteral("SVGLoad")  ||
>+                      eventType.EqualsLiteral("SVGAbort") ||
>+                      eventType.EqualsLiteral("SVGError"),
>+                      "Received unexpected event.");

Drop trailing period -----------------------------^

>+void
>+VectorImage::OnSVGDocumentLoaded()
>+{

I'm pretty sure we strictly expect OnSVGDocumentParsed() to be called before this method, right?

If so, we can & should probably assert that mParseCompleteListener is null at the beginning of this method. (That's just an indicator for "OnSVGDocumentParsed was called".)

>+  if (!mSVGDocumentWrapper->ParsedSuccessfully()) {
>+    OnSVGDocumentError();
>+    return;
>+  }

This check (and the "ParsedSuccessfully" method itself, actually) are now redundant.

ParsedSuccessfully() is just a wrapper for GetRootSVGElem(), and you added a separate (and earlier) check for that in OnSVGDocumentParsed().  So we don't need to check it again here, and we can just remove the ParsedSuccessfully() method entirely from SVGDocumentWrapper.

>+VectorImage::OnSVGDocumentError()
>+{
[...]
>+  RefPtr<imgDecoderObserver> observer(mObserver);
>+  if (observer)
>+    observer->OnStopDecode(NS_ERROR_FAILURE); // Unblock page load.
> }

I'd prefer to have braces around this clause, but I won't hold you to that.  (This happens somewhere else in the patch, too.)

r=me with the above.  Thanks again for fixing this bug!

Comment 36

[Seth Fowler [:seth] [:s2h]]

Attachment: Decouple OnStopRequest and OnStopDecode for VectorImage.

Thanks for the review, Daniel. I've made the changes you requested. New try job here: https://tbpl.mozilla.org/?tree=Try&rev=74c1bf0b12e8

Comment 37

[Daniel Holbert [:dholbert]]

That try run had two related-looking test failures, on Win Debug:

First orange: https://tbpl.mozilla.org/php/getParsedLog.php?id=19223020&tree=Try
{
REFTEST TEST-START | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/backgrounds/vector/tall--auto-32px--omitted-width-percent-height.html | 866 / 8986 (9%)
###!!! ABORT: Shouldn't have the parse listener: '!mParseCompleteListener', file e:/builds/moz2_slave/try-w32-dbg/build/image/src/VectorImage.cpp, line 859
}

(Is that the new assertion I suggested? I guess you were right that those notifications can arrive in either order, though that seems a bit odd.

Perhaps we can just ditch the parse listener as soon as we receive the onload notification?


Second orange: https://tbpl.mozilla.org/php/getParsedLog.php?id=19222633&tree=Try
{
13148 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/image/test/mochitest/test_animSVGImage.html | timing out after 120000ms.  Animated image still doesn't look correct, after call #1854 to onStopFrame
}
That one sounds like we're not animating correctly.

Comment 38

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #37)
> Perhaps we can just ditch the parse listener as soon as we receive the
> onload notification?

(er s/onload/SVGLoad/)

Comment 39

[Seth Fowler [:seth] [:s2h]]

Truthfully I am rather puzzled that they can arrive in either order with the current code, though that seems to be what's going on here. I'm going to experiment a bit and see if I can understand what's going on here.

Comment 40

[Seth Fowler [:seth] [:s2h]]

Earlier today I launched a try run with some printf logging to determine the order that the events arrive. And sure enough:

https://tbpl.mozilla.org/php/getParsedLog.php?id=19257381&tree=Try

> [704059] SVGLoadEventListener got HandleEvent
> [704059] OnSVGDocumentLoaded is being called
> [704059] OnSVGDocumentLoaded would have asserted because mParseCompleteListener is non-null
> [704059] SVGLoadEventListener is cancelling itself in HandleEvent
> [704059] SVGParseCompleteListener got EndLoad
> [704059] OnSVGDocumentParsed is being called
> ###!!! ABORT: Should still have the listeners: 'mLoadEventListener'

As you can see I disabled the assertion that triggered in the previous try run, and put that "would have asserted" message instead.

For some reason this always happens on Windows. Really weird.

Comment 41

[Seth Fowler [:seth] [:s2h]]

So it sounds like what we want is:

- OnSVGDocumentParsed does not cancel anything itself. If there's no root SVG tag, it calls OnSVGDocumentError (which cancels both listeners).
- Both OnSVGDocumentError and OnSVGDocumentLoaded cancel SVGParseCompleteListener _and_ SVGLoadEventListener unconditionally.

I had something a bit more complex here originally, but I think it's simplest to do the cancellation in only two places. It can never happen that neither OnSVGDocumentError nor OnSVGDocumentLoaded ends up being called.

Comment 42

[Seth Fowler [:seth] [:s2h]]

Attachment: Decouple OnStopRequest and OnStopDecode for VectorImage.

OK, here's a new patch. This actually simplifies the code slightly. Hopefully it works. =)

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=9521e12af6a6

Comment 43

[Seth Fowler [:seth] [:s2h]]

Daniel, what do you think of the try situation here? Is this patch landable right now?

Comment 44

[Daniel Holbert [:dholbert]]

We're probably good to land it -- the reftest & mochitest-oth oranges are known-intermittent, but they're related to your changes, so it's worth considering the possibility that those instances there are "real".

I triggered a few more runs of those testers to get an idea of their frequency. As long as it's not at like 50% failure rate, I think we're good to consider those intermittent & land this. (woot!)

Comment 45

[Daniel Holbert [:dholbert]]

Hmm... unfortunately, the Fedora Debug M-oth failure from comment 42's Try run (test_animSVGImage.html) is looking worrisome, with failures in 5 out of 6 runs so far on that platform. :-/

(Note that its randomorange bug was filed  4 days ago, and it only has 2 reports (outside of this Try run) so far -- both of which are for WinXP opt builds.  So I think this high-frequency failure on Fedora-debug is new, rather than just a manifestation of the existing randomorange.  At best, we're tickling some preexisting race condition in a new & spammy way.)

Ideally, we should understand what's going on there (maybe w/ some logging) before we land this.

Comment 46

[Seth Fowler [:seth] [:s2h]]

Yeah, I agree. I'm taking a look right now.

Comment 47

[Seth Fowler [:seth] [:s2h]]

Attachment: Decouple OnStopRequest and OnStopDecode for VectorImage.

Updated patch which depends on bug 837315. Not requesting review yet; let's see how try goes and whether bug 837315 gets a r+. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=9cde61d36e8b

Comment 48

[Seth Fowler [:seth] [:s2h]]

I'm running an experimental try job here:

https://tbpl.mozilla.org/?tree=Try&rev=90ed185a2963

This builds on the previous experimental patch by moving the firing of the |load| event for images from OnStopRequest to the first call to OnStopFrame (in other words, after one frame of the image has been decoded). In the case of SVGs, we fire OnStopFrame right before OnStopDecode, so this happens after everything is loaded and immediately before unblocking the page's onload. In local testing this resolves the oranges from the last try run. Hopefully it doesn't produce a ton of new ones =)

Comment 49

[Seth Fowler [:seth] [:s2h]]

A single missing character botched that entire try push. Sigh. New job here:

https://tbpl.mozilla.org/?tree=Try&rev=61541467ca72

Comment 50

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Decouple OnStopRequest and OnStopDecode for VectorImage.

Updated patch, incorporating all review comments up to this point. Rereview
isn't required. This patch ensures that the document containing an SVG image
fires its |load| event only when the SVG image is fully loaded, including all of
its resources. The changes in behavior expose issues with the |img| element load
event, though, so this patch by itself causes oranges. The oranges are fixed in
part 2.

Comment 51

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Let images call imgStatusTracker::OnStopRequest.

This patch gives responsibility for calling imgStatusTracker::OnStopRequest to
the Image classes themselves, as part of OnImageDataComplete. RasterImage calls
imgStatusTracker::OnStopRequest immediately, but VectorImage defers calling it
until its internal SVG document is fully loaded or we know that it won't load
successfully. This prevents oranges introduced in the first patch and generally
solves random oranges that were related to things like querying the width or
height of an SVG image in an |img| element's onload handler.

Comment 52

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Reenable tests that were orange due to SVG loading issues.

Reenable the tests that should now be fixed. (The ones blocked by this bug.)
Let's verify that they're all working OK now.

Comment 53

[Seth Fowler [:seth] [:s2h]]

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=ab630c324e1f

Comment 54

[Joe Drew (not getting mail)]

Comment on attachment 713759 [details] [diff] [review]
(Part 2) - Let images call imgStatusTracker::OnStopRequest.

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

::: image/src/VectorImage.cpp
@@ +382,5 @@
> +    finalStatus = aStatus;
> +
> +  // If there's an error already, we need to fire OnStopRequest on our
> +  // imgStatusTracker immediately. We might not get another chance.
> +  if (mError) {

This might need to be mError || NS_FAILED(finalStatus)

::: image/src/VectorImage.h
@@ +81,5 @@
>  
>  private:
> +  // A private structure used for storing the parameters to
> +  // imgStatusTracker::OnStopRequest until we're ready to call it.
> +  struct StopRequest

Maybe name this StopRequestArgs?

@@ +98,5 @@
>    nsRefPtr<SVGRootRenderingObserver> mRenderingObserver;
>    nsRefPtr<SVGLoadEventListener>     mLoadEventListener;
>    nsRefPtr<SVGParseCompleteListener> mParseCompleteListener;
>  
> +  StopRequest    mStopRequest;            // If we need to fire OnStopRequest,

What do you think of making this an nsAutoPtr? I did a similar thing in some of my patches, and it encodes the boolean in whether or not you've set the pointer to a new StopRequest.

::: image/src/imgRequest.cpp
@@ +635,5 @@
>      if (NS_FAILED(rv) && NS_SUCCEEDED(status))
>        status = rv;
>    }
>  
> +  if (!mImage) {

this can be "else"

@@ +639,5 @@
> +  if (!mImage) {
> +    // We have to fire imgStatusTracker::OnStopRequest ourselves because there's
> +    // no image capable of doing so.
> +    imgStatusTracker& statusTracker = GetStatusTracker();
> +    statusTracker.RecordCancel();  // No image indicates an error!

It's a little weird that you're recording cancel above the call to Cancel, below.

Comment 55

[Seth Fowler [:seth] [:s2h]]

Thanks for the review, Joe!

(In reply to Joe Drew (:JOEDREW! \o/) from comment #54)
> @@ +98,5 @@
> >    nsRefPtr<SVGRootRenderingObserver> mRenderingObserver;
> >    nsRefPtr<SVGLoadEventListener>     mLoadEventListener;
> >    nsRefPtr<SVGParseCompleteListener> mParseCompleteListener;
> >  
> > +  StopRequest    mStopRequest;            // If we need to fire OnStopRequest,
> 
> What do you think of making this an nsAutoPtr? I did a similar thing in some
> of my patches, and it encodes the boolean in whether or not you've set the
> pointer to a new StopRequest.

It's definitely nice to tie the two together, but I hate to use dynamic allocation
for an object that's so tiny and which virtually every VectorImage will end up needing.
I think the best option here might be Maybe<T>; I'll switch to that.

> @@ +639,5 @@
> > +  if (!mImage) {
> > +    // We have to fire imgStatusTracker::OnStopRequest ourselves because there's
> > +    // no image capable of doing so.
> > +    imgStatusTracker& statusTracker = GetStatusTracker();
> > +    statusTracker.RecordCancel();  // No image indicates an error!
> 
> It's a little weird that you're recording cancel above the call to Cancel,
> below.

Yes, but it's needed in this case because sometimes the situation that causes an image
to never be created does not set any error flags on the imgStatusTracker. Without this
we were triggering onload instead of onerror on the img element in those cases. The right
fix here is almost certainly to ensure that every situation that causes an image not to be
created sets an error on the imgStatusTracker before this point, but I'd prefer to debug
that in a separate bug so this can get landed.
bug.

Comment 56

[Seth Fowler [:seth] [:s2h]]

Regarding the try job above, 100% of the failures are caused by reenabling the test for bug 629885. That's not entirely surprising as things seem to have changed since that bug was filed (the test case is different now). It would be good to triage what's going on there, as that bug might just be obsolete, but the right fix here is just not to reenable it.

Comment 57

[Seth Fowler [:seth] [:s2h]]

New try job here incorporating the review changes above and fixing the issue with the wrongly enabled/changed test. Additionally, I remove 'skip-if(B2G)' on two of the tests and added B2G to the try job, as I suspect we don't need that anymore. Updated patches will follow shortly.

https://tbpl.mozilla.org/?tree=Try&rev=8c1f7a5d7507

Comment 58

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Decouple OnStopRequest and OnStopDecode for VectorImage.

Updated part 1. This is rebased on top of bug 840841 and incorporates the review changes above.

Comment 59

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Let images call imgStatusTracker::OnStopRequest.

Updated part 2, also rebased and with review changes.

Comment 60

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Reenable tests that were orange due to SVG loading issues.

Updated part 3, with changes as detailed above. (Bug 629885's test is now unaffected by this patch, and all 'skip-if(B2G)'s on affected tests are removed.)

Comment 61

[Seth Fowler [:seth] [:s2h]]

Try looks good. Let's land this thing.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/184454414a01
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d1fc7d753c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b7d9acf0d5b

\o/

Comment 63

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/184454414a01
https://hg.mozilla.org/mozilla-central/rev/e6d1fc7d753c
https://hg.mozilla.org/mozilla-central/rev/6b7d9acf0d5b

Comment 64

[Ed Morley [:emorley]]

(In reply to Ryan VanderMeulen [:RyanVM][Away 22-Mar to 25-Mar] from comment #63)
> https://hg.mozilla.org/mozilla-central/rev/6b7d9acf0d5b

This undid the random-if for WinXP for the test in bug 703806 - for which the intermittent isn't fixed :-(

Comment 65

[Seth Fowler [:seth] [:s2h]]

Yeah, I believed that it fixed it at the same, but subsequent experience has shown otherwise. =(