621918-2.svg doesn't seem to load its remote filter correctly

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment)

Preface: My first landed patch for bug 621253 had a silly bug that made us crash on testcases that use filters that don't load correctly.

621918-2-ref.svg was one of the files tests that had a crash.  From some GDB'ing, I've confirmed that its resource document doesn't seem to get loaded correctly (at least not initially), though I'm not sure why.

From trapping the crash in gdb and going back in time a little bit, what happens is:
 - We enter nsExternalResourceMap::PendingLoad::OnStartRequest
   -- That calls SetupViewer
     --- That takes the first failure 'return' statement, due to a failure
         in httpChannel->GetRequestSucceeded().
   -- So we don't have a nsIDocumentViewer set up for our external resource
   -- So when we call "map.AddExternalResource", that skips the whole "if (aViewer)" case, and doesn't do much of anything.  (except crashing with my silly first patch from bug 621253)

I think this indicates that the filter's resource-document isn't loading correctly, particularly since the GetRequestSucceeded failure makes us hit this comment & error return code:
>      // Bail out on this load, since it looks like we have an HTTP error page
>      return NS_BINDING_ABORTED;

From quickly grepping for other uses of filters.svg, it looks like we might need a special <use> element in order to make filters.svg loads before we take the reftest snapshot.  I'm not sure if that would help here....
Depends on: 621918
Hmm.  What was the HTTP channel status in this case?
If you mean aRequest->mResponseHead->Status() (since aRequest is a nsHttpChannel):  dbaron suggested looking at that, but at the time of the crash, aRequest->mResponseHead is actually null.
Interesting.  So it never got a response from the server at all?

What's aRequest->mStatus ?
(In reply to comment #3)
> What's aRequest->mStatus ?
(gdb) p/x aRequest->mStatus
$1 = 0x804b0002

http://twpol.dyndns.org/mozilla/misc/nserror?0x804B0002 says that code means:
>  NS_BINDING_ABORTED
>  The async request failed because it was aborted by some user action.

Also, just as a sanity-check to prove that this is the right external resource, here's the URL associated with the channel:
(gdb) p aRequest->mSpec.mData
$3 = 0x47fe7188 "http://localhost:4444/1295076957181/1/filters.svg"
Ok, so I placed a breakpoint in nsHttpChannel::Init, and here's what I got:
(1)  nsHttpChannel::Init for with uri="[...]621918-2.svg"
(2)  nsHttpChannel::Init for with uri="[...]filters.svg"
   * At this point I put a watchpoint on this nsHttpChannel's "mStatus" var
(3) watchpoint triggered, in a call to nsLoadGroup::Cancel(NS_BINDING_ABORTED)
    (I'll attach the stack for this in case it's useful)
(4)  nsHttpChannel::Init for with uri="[...]621918-2-ref.svg"
(5)  nsExternalResourceMap::PendingLoad::OnStartRequest for the filter, with...
  mDisplayDocument->mRawPtr->mDocumentURI.mRawPtr->mSpec == "[...]621918-2.svg"

(and then we crash before completing the OnStartRequest call, in the code added by bug 621253's first patch)

Note that, while the console message going by is about the reference case...
> REFTEST TEST-START | http://localhost:4444/1295078756457/1/bugs/621918-2-ref.svg
...in fact the last point above (in (5)) shows that we're creating the external resource document for the *testcase* file when this all fails.
Summary: 621918-2-ref.svg doesn't seem to load its remote filter correctly → 621918-2.svg doesn't seem to load its remote filter correctly
(In reply to comment #5)
> (3) watchpoint triggered, in a call to nsLoadGroup::Cancel(NS_BINDING_ABORTED)
>     (I'll attach the stack for this in case it's useful)

Here's that stack.  The point in the stack where we seem to change from "everything's ok" to "aborting" seems to be when nsDocShell::InternalLoad calls Stop(nsIWebNavigation::STOP_NETWORK), here:
http://hg.mozilla.org/mozilla-central/annotate/3d4620449437/docshell/base/nsDocShell.cpp#l8438
Sounds like the test doesn't wait for the external resource to load before moving on to the next test.  And in particular, the external resource load starts from _painting_ and we don't paint before firing onload, and onload is what the harness watches for by default.

roc, would making this testcase wait for an invalidate event solve that problem?  I'm not sure when the reftest invalidate thing fires, compared to painting.
In a lot of (all?) other testcases that reference filters.svg, there's a special
>  <use xlink:href="../filters.svg#empty">
element somewhere, with a comment about that helping the external resource to load before the document's onload.

Is this bug precisely what that <use> hack is intended to fix?
How can we wait for external resource loads to complete? I can't think of any way.
> Is this bug precisely what that <use> hack is intended to fix?

I _think_ so.

> How can we wait for external resource loads to complete?

Right now there is no good way.  We can come up with various hacks (e.g. if the page does something before the end of parsing or off of DOMContentLoaded that triggers a drawWindow that will paint and hence start the load, and then onload will wait for the external resource), though.  For example, the reftest harness could watch DOMContentLoaded and check for some class on the root element to see whether it needs to call drawWindow at that point.  Worth it?
Painting on DOMContentLoaded won't help if the external resource is referenced from an external style sheet, I guess. Any better ideas? :-)
Painting in a <script> tag that comes right before </body>?  That will wait for stylesheets to load before running...
That sounds good. We'll need a hook to synchronously call back to the reftest harness at that point. I guess relying on mutation events to be synchronous isn't a good idea. Maybe we can dispatch a MozReftestSyncPaint event and have the reftest harness handle that by doing a drawWindow and throwing away the results?
That's along the lines of what I was thinking, yes.
In the long run, fixing bug 277955 would provide a neat way to fix this, right?
So how would we fix 277955? Flush style, layout and invalidate before firing the 'load' event, if any element has externalResourcesRequired="true"?
Per spec, externalResourcesRequired="true" is meant to delay the load event being dispatched to the element it appears on until any resource referenced by anything in that subtree is available.  (Since we only dispatch load to the root element, that should simplify things a little.)  It's also meant to disable rendering of that subtree until the resources are available.

Would "flush style, layout and invalidate" do that waiting?
Flushing style and layout and forcing a paint (which we can maybe just discard the results of) before firing 'load' would ensure that all currently-needed external resource loads are triggered. Those loads would then delay the document load event until they're done.
Per bug 625290 comment 13, this reftest & its reference case have both been rendering to blank pages most of the time, due to the filters not being loaded. (somewhat unsurprisingly, given this bug).

To make the test useful (and fix its randomorange bug from occasional successful-filter-loading), I added the <use> hack that I mentioned in comment 8 here.  That seems to fix it (at least, it did on TryServer last night).  That push to m-c was noted in Bug 625290 comment 14.

So, I'm closing this bug, since 621918-2.svg should now be loading its remote filter correctly.

Bug 277955 can track any work on implementing "externalResourcesRequired", brought up in comment 15-19 here. (Hopefully that can replace the filters.svg#empty hack entirely, in filters testcases across the whole tree, once it's implemented.)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.