Closed Bug 582840 Opened 10 years ago Closed 10 years ago

Remove nsIWebProgressListener(2) support from TabChild and TabParent

Categories

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

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+
fennec 2.0+ ---

People

(Reporter: mfinkle, Assigned: benedict)

References

Details

Attachments

(2 files, 4 obsolete files)

Since bug 514705 is a WONTFIX, I think we should consider removing the existing code in TabChild and TabParent which supports webprogress listeners. The code is not being used and is still actively sending IPC messages.
Agreed.
blocking2.0: --- → betaN+
Component: IPC → DOM
QA Contact: ipc → general
I can take this. Good chance to learn about hg backout and killing your darlings.
Assignee: nobody → bhsieh
Attached patch and merge (obsolete) — Splinter Review
Just throwing those up there for now, the merge looks a bit suspicious (though it compiles and runs test-ipc?). It will probably be easier to just create a new patch that rips out the bits manually, instead of this backout. Not sure if one of these is the 'right' approach.
Attachment #461727 - Attachment is obsolete: true
Attachment #461728 - Attachment is obsolete: true
Attachment #463019 - Flags: review?(Olli.Pettay)
I also have a test for non-remote webprogress. Not sure if I should file that separately or here, since I did muck with non-remote webprogress while implementing (and reverting) this stuff.
test
Comment on attachment 463019 [details] [diff] [review]
removes webprogress from tabparent and tabchild


> [scriptable, uuid(65d2c9e2-852c-48cf-a95d-9b82f1273c15)]
> interface nsIFrameLoader : nsISupports
Update the uuid

> NS_INTERFACE_MAP_BEGIN(TabChild)
>-  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIWebProgressListener2)
>   NS_INTERFACE_MAP_ENTRY(nsIWebBrowserChrome)
>   NS_INTERFACE_MAP_ENTRY(nsIWebBrowserChrome2)
>   NS_INTERFACE_MAP_ENTRY(nsIEmbeddingSiteWindow)
>   NS_INTERFACE_MAP_ENTRY(nsIEmbeddingSiteWindow2)
>   NS_INTERFACE_MAP_ENTRY(nsIWebBrowserChromeFocus)
>   NS_INTERFACE_MAP_ENTRY(nsIInterfaceRequestor)
>   NS_INTERFACE_MAP_ENTRY(nsIWindowProvider)
>-  NS_INTERFACE_MAP_ENTRY(nsIWebProgressListener)
>-  NS_INTERFACE_MAP_ENTRY(nsIWebProgressListener2)
>-  NS_INTERFACE_MAP_ENTRY(nsSupportsWeakReference)
You remove support for weak reference here...


> class TabChild : public PBrowserChild,
>-                 public nsIWebProgressListener2,
>                  public nsIWebBrowserChrome2,
>                  public nsIEmbeddingSiteWindow2,
>                  public nsIWebBrowserChromeFocus,
>                  public nsIInterfaceRequestor,
>                  public nsIWindowProvider,
>                  public nsSupportsWeakReference,
>                  public nsIDialogCreator,
>                  public nsITabChild
...but not here. 
Bug 514705 added the support for weakref, so I think it can be safely removed in both places.
(or if we need it for something, we shouldn't remove it in either of place)
Attachment #463019 - Flags: review?(Olli.Pettay) → review+
Duplicate of this bug: 581930
Blocks: 581532
Blocks: 583135
Latest trunk also contain nsISecureBrowserUI, which is depend from mSecurityState, and mSecurityState updated in SecurityChanged notification which is supposed to be removed in this bug... 

Should we remove nsISecureBrowserUI interface too ? because that is coming from bug 536301
Blocks: 536301
(In reply to comment #11)
> Latest trunk also contain nsISecureBrowserUI, which is depend from
> mSecurityState, and mSecurityState updated in SecurityChanged notification
> which is supposed to be removed in this bug... 
> 
> Should we remove nsISecureBrowserUI interface too ? because that is coming from
> bug 536301

Oh crap. It would be nice to keep nsISecureBrowserUI working. We have a makeshift version working in Fennec now. At least I think it is working.
(In reply to comment #12)
> We have a makeshift version working in Fennec now. At least I think it is working.

Mark, can you confirm it?

It's not much clear from bug 568502 if this is used or not.  I clearly have to get more familiar with mobile browser.. but, if I read correctly, from some browsing through the code it seems that browser collects/updates data in webProgress notifications using the regular, unchanged, secureBrowserUI implementation and sends them to the chrome browser in a way to me similar as tab child and parent do in c++.
(In reply to comment #13)
> (In reply to comment #12)
> > We have a makeshift version working in Fennec now. At least I think it is working.
> 
> Mark, can you confirm it?
> 
> It's not much clear from bug 568502 if this is used or not.  I clearly have to
> get more familiar with mobile browser.. but, if I read correctly, from some
> browsing through the code it seems that browser collects/updates data in
> webProgress notifications using the regular, unchanged, secureBrowserUI
> implementation and sends them to the chrome browser in a way to me similar as
> tab child and parent do in c++.

Actually, we have problems with our makeshift version. See bug 575950.

Is there a way to keep enough of the progress listener for secureBrowserUI to remain functional? Or can we get the secureBrowserUI working with explicit messages.
We should figure out if bug 575950 is caused by something we miss that should be done in bug 568502 or there is a problem elsewhere.

I have never build nor run Fennec my self, but it seems to be the time to start with it.  I'll try to debug it and let you know what we can do.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
Can we just apply this patch with secure UI drop and then fix secure UI stuff separately?
Attachment #463019 - Attachment is obsolete: true
Attachment #473020 - Flags: review?(Olli.Pettay)
(In reply to comment #16)
> Created attachment 473020 [details] [diff] [review]
> Updated to latest m-c 360f315ba90a, fixed review comments
> 
> Can we just apply this patch with secure UI drop and then fix secure UI stuff
> separately?

I would be OK with that. Bug 575950 is tracking a fix for the secureUI issue. There might be other bugs too.

Honza - What do you think?
(In reply to comment #17)

With the latest patch there will be no security state tracking for UI under multiprocess Firefox, but we can live with that.

If there are any consumers in Fennec that needs nsISecureBrowserUI implementation on the _chrome_ process, we simply have to create a new class that implements it and receives security state changes from content.

> Bug 575950 is tracking a fix for the secureUI issue.

Not sure what states for "the secureUI issue" here.  The bug is tracking a different problem: we are not able to send security info object using a serialized string between chrome and content process after NSS has been turned off on the child process.

> There might be other bugs too.

Agree.  For me the patch from bug 575950 fixes security state notification sent to the chrome process, but it uncovers or causes a strange bug on Fennec.

BTW: I was not able to build Fennec, I'm getting internal compiler errors on my VPC CentOS 5, so I cannot help with it.

> 
> Honza - What do you think?

So, I would land this patch and if we are not getting the security UI on Fennec correctly filled, let's fix it in a new bug.
Waiting for smaug to review, but we should land this when ready and fix the SSL issues in a separate bug.
Attachment #473020 - Flags: review?(Olli.Pettay) → review+
http://hg.mozilla.org/mozilla-central/rev/7eb93fe05d3a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Oh, fix was reverted due to oranges
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated patch.

The backout of this patch was apparently unneeded - the push right before it caused the error it was blamed for. Tinderbox only ran 1/4 of the linux runs for the previous push, so it was hard to notice (maybe could only be seen after the backout...) - but it fails the error that happened in all the linux runs of this patch, that prompted the backout.

Pushed the updated patch to try, waiting to see it's green to make sure.
Attachment #473020 - Attachment is obsolete: true
Tryserver confirms it, this patch is good to go.
Pushed:

http://hg.mozilla.org/mozilla-central/rev/6ec3a45f4309
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.