Closed Bug 1336789 Opened 4 years ago Closed 4 years ago

Remove DeprecatedBlockingResolve from mailnews/base/util/nsMsgUtils.cpp

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 791645

People

(Reporter: Paenglab, Unassigned)

References

Details

Bug 778201 removed the DeprecatedBlockingResolve and building TB fails on line https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgUtils.cpp#2147.

Baking-out this bug makes TB building again.
https://hg.mozilla.org/comm-central/rev/cedd5f2f7fa64aeb7eae002f4b1ee7503c938d35

Landed immediate bustage fix. As I see it, MsgExamineForProxy() and all its callers should be removed.
Oops, that left an unused variable behind. Ah well, it will build on Windows which is more forgiving ;-)
Do we agree that removal of MsgExamineForProxy() is the way to go? There doesn't seem to be a replacement  for DeprecatedBlockingResolve().
Flags: needinfo?(rkent)
Flags: needinfo?(mkmelin+mozilla)
That wasn't a complete success at all since it caused may test failures.

Jed and Patrick, can you advise us here, please. How should we replace DeprecatedBlockingResolve()?
Flags: needinfo?(mcmanus)
Flags: needinfo?(jld)
I tried to move DeprecatedBlockingResolve() to C-C. Sadly I ended up needing to copy
Resolve_Internal(), SetupPACThread(), GetProtocolInfo(), GetProxyURI() and there is still no end in sight with an estimated 50+ compile errors asking for more.
Flags: needinfo?(rkent)
Flags: needinfo?(mkmelin+mozilla)
You need to use the asynchronous versions - the blocking one can seriously jank your app depending on the resolution that might need to be done (which can require network calls).
Flags: needinfo?(mcmanus)
Can you point me to an example, please.
Flags: needinfo?(jld)
Flags: needinfo?(mcmanus)
I thin(In reply to Patrick McManus [:mcmanus] from comment #6)
> You need to use the asynchronous versions
I think that's easier said then done, especially in a Sunday afternoon bustage situation. I found:
https://hg.mozilla.org/releases/mozilla-esr24/rev/dcae72a1333c
whose commit comment says:
  move proxy resolution to separate thread and remove sync
however, at first glance I don't see and sync to async conversion in there.

If I understand it correctly, any sync to async conversion entails a complete restructuring of your program logic.
https://hg.mozilla.org/comm-central/rev/41590e369c5c6cae8a892fd0238c456ecf9da69a
This fixes the compile error on Linux and Mac by making MsgExamineForProxy() return an error.
With this change, TB will run on all platforms, but all proxy functionality is lost.

I think it will take time to fix this bug, so it's better to be limping along for a while than being completely broken. We can't afford to stop all landings and all development due to the unannounced withdrawal of an M-C interface.
(In reply to Jorg K (GMT+1) from comment #9)
> I think it will take time to fix this bug, so it's better to be limping
> along for a while than being completely broken. We can't afford to stop all
> landings and all development due to the unannounced withdrawal of an M-C
> interface.

For all the trouble this may be causing, I don't think removing a function that has the word 'deprecated' even in its name since 2012 counts as "unannounced" ;)
Unannounced: No announcement was made. Or did I miss something?

In comparison, cache(1) is being phased out slowly so we had the time to switch to cache2 at our own pace without any bustage. I'd say that this problem here is harder than the transition to cache2 which was basically an editing job.
(In reply to Jorg K (GMT+1) from comment #11)
> Unannounced: No announcement was made. Or did I miss something?

This is just another case of TB technical debt. After all, someone in c-c added that call to a clearly deprecated function, probably as a short-term bustage fix.

And indeed, if we look at the hg history we find https://bugzilla.mozilla.org/show_bug.cgi?id=791645#c0: "Bug 791457 was a stop gap measure. DeprecatedBlockingResolve will be going away as well once the java plugin code is rewritten to use AsyncResolve. We need to make our code use the async path before that happens." That was 4 years ago ;)

The removal of NSAPI plugin support was announced for 52, this is merely followup dead code cleanup as far as m-c is concerned.
(In reply to Jorg K (GMT+1) from comment #11)
> I'd say that this problem here is harder than the transition to cache2 which was basically an
> editing job.

By the way, reading more of bug 791645 agrees with your expectation that fixing this is not trivial...
(In reply to aleth [:aleth] from comment #13)
> By the way, reading more of bug 791645 agrees with your expectation that
> fixing this is not trivial...

...there does seem to be a WIP for an async solution in there too though, with lots of comments by jcranmer.
(In reply to Jorg K (GMT+1) from comment #7)
> Can you point me to an example, please.

test_protocolproxyservice.js
Flags: needinfo?(mcmanus)
OK, let's all head over there. I'll see whether that patch still applies.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 791645
You need to log in before you can comment on or make changes to this bug.