Closed Bug 1055728 Opened 5 years ago Closed 5 years ago

Investigate why mochitests reporting that retargeting to HTML parser thread is failing.

Categories

(Core :: Networking, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jduell.mcbugs, Unassigned)

References

Details

Attachments

(1 file)

https://tbpl.mozilla.org/php/getParsedLog.php?id=46282126&full=1&branch=try#error0

Search for "retarget" in this log and you'll see a lot of messages about retargeting to the HTML parser failing.   

This is not the cause of the error in the log--just an FYI that I noticed this.  I'd like us to make sure that retargeting is working in the common case--IIRC we never landed any test to detect if retargeting starts breaking.

Do we have any idea of what common reasons for why retargeting fails?
FYI, during my work on backtrack I've spot that we don't deliver HTLM data to the parser thread but rather to the main thread *from time to time*.
(In reply to Honza Bambas (:mayhemer) from comment #1)
> FYI, during my work on backtrack I've spot that we don't deliver HTLM data
> to the parser thread but rather to the main thread *from time to time*.

After refreshing my C-C source tree after three months or so,
I got a lot of this new warning during the testing of DEBUG version of TB using its |make mozmill| test suite.

So this is a new warning and does not seem to be right.

Shouldn't this code in
http://mxr.mozilla.org/comm-central/source/mozilla/parser/html/nsHtml5StreamParser.cpp#938

be modified so that the
line 946-948 (that outputs the warning) be inside the if() { } in
942-944?

 
938   // Attempt to retarget delivery of data (via OnDataAvailable) to the parser
939   // thread, rather than through the main thread.
940   nsCOMPtr<nsIThreadRetargetableRequest> threadRetargetableRequest =
941     do_QueryInterface(mRequest, &rv);
942   if (threadRetargetableRequest) {
943     rv = threadRetargetableRequest->RetargetDeliveryTo(mThread);
944   }
945 
946   if (NS_FAILED(rv)) {
947     NS_WARNING("Failed to retarget HTML data delivery to the parser thread.");
948   }
949 

I mean if this threadRegargetableRequest is false,
rv on line 946 picks up a much earlier value and it does not seem very correct.
Flags: needinfo?(jduell.mcbugs)
Sorry I meant to have the feedback from the author of these lines: according to
http://hg.mozilla.org/mozilla-central/rev/9a02ba6359bb
it is Kyle Huey.
Flags: needinfo?(khuey)
No, the code is correct.  do_QueryInterface will overwrite the result value[0].  The warning is intended to warn in two cases:

1. When you are loading HTML over a protocol that does not support retargeting to the parser thread (e.g. ftp, http in a child process, etc).
2. When you are loading HTML over a protocol that does support retargeting to the parser thread but that operation fails because there is a streamlistener in the chain that does not support retargeting (e.g. loading HTML via XHR).

[0] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCOMPtr.cpp#27
Flags: needinfo?(khuey)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> No, the code is correct.  do_QueryInterface will overwrite the result
> value[0].  The warning is intended to warn in two cases:
> 
> 1. When you are loading HTML over a protocol that does not support
> retargeting to the parser thread (e.g. ftp, http in a child process, etc).
> 2. When you are loading HTML over a protocol that does support retargeting
> to the parser thread but that operation fails because there is a
> streamlistener in the chain that does not support retargeting (e.g. loading
> HTML via XHR).
> 
> [0] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCOMPtr.cpp#27

Thank you for the clarification.
But, I am not familiar with the code: but, so basically,
the explanation is that 
when do_QueryInterface() succeeds (and rv is NS_OK, I suppose)
there is no chance of the expression in
"if (threadRetargetableRequest) " taken to be false: that is the code and configuration of
the current code base 
is such that if do_QueryInterface succeeds and rv is OK, then if(...) { rv = } is  always executed?
[I am a little confused when I saw these messages in TB testing code, but
there are code in TB that access external database using HTTP, so that must be it.
Whether TB's HTTP code is configured to make the assumptions above correct is 
something I don't know. ]

OK, it dawned on me, basically do_QueryInterface() returns something non-false when it succeeds (rv == NS_OK) and  so if (rv == NS_OK && threadRetargetableRequest) can be shortened to 
if (threadRetargetableRequest). Right? I hope TB codes does not return nullptr here, but maybe that is happening here.

Working on a large existing code base to report a bug here and there from the user's point of view 
takes time to learn.

TIA
If we're issuing this warning for every single HTML resource when we're using e10s, that's probably too verbose?  Henri, what do you think?
Flags: needinfo?(jduell.mcbugs) → needinfo?(hsivonen)
I think we need to fix bug 1015466 before we ship e10s ;)
(In reply to Jason Duell (:jduell) from comment #6)
> If we're issuing this warning for every single HTML resource when we're
> using e10s, that's probably too verbose?  Henri, what do you think?

We probably shouldn't be emitting warnings, then.
Flags: needinfo?(hsivonen)
I see no point in issuing a spurious warning over and over again.  This patch represses the warning when we're not on the parent process, since we don't support off-main retargeting in child processes yet.
Attachment #8507119 - Flags: review?(hsivonen)
Attachment #8507119 - Flags: review?(hsivonen) → review+
Was missing the right #include:
 
  https://hg.mozilla.org/integration/mozilla-inbound/rev/08b95f8fe876
Flags: needinfo?(jduell.mcbugs)
https://hg.mozilla.org/mozilla-central/rev/08b95f8fe876
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
See Also: → 1184675
Assignee: sworkman → nobody
You need to log in before you can comment on or make changes to this bug.