582840 - Remove nsIWebProgressListener(2) support from TabChild and TabParent

Status: RESOLVED FIXED

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

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 1

[Benjamin Smedberg]

Agreed.

Comment 2

[Benedict Hsieh [:benedict]]

I can take this. Good chance to learn about hg backout and killing your darlings.

Comment 3

[Benedict Hsieh [:benedict]]

Attachment: backout of 40031, which implemented

Comment 4

[Benedict Hsieh [:benedict]]

Attachment: and merge

Comment 5

[Benedict Hsieh [:benedict]]

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.

Comment 6

[Benedict Hsieh [:benedict]]

Attachment: removes webprogress from tabparent and tabchild

Comment 7

[Benedict Hsieh [:benedict]]

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.

Comment 8

[Benedict Hsieh [:benedict]]

Attachment: test for non-remote

test

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 11

[Oleg Romashin (:romaxa)]

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

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 13

[Honza Bambas (:mayhemer)]

(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++.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 15

[Honza Bambas (:mayhemer)]

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.

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: 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?

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

(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?

Comment 18

[Honza Bambas (:mayhemer)]

(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.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Waiting for smaug to review, but we should land this when ready and fix the SSL issues in a separate bug.

Comment 20

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/7eb93fe05d3a

Comment 21

[Oleg Romashin (:romaxa)]

Oh, fix was reverted due to oranges

Comment 22

[Alon Zakai (:azakai)]

Attachment: Updated to latest m-c

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.

Comment 23

[Alon Zakai (:azakai)]

Tryserver confirms it, this patch is good to go.

Comment 24

[Alon Zakai (:azakai)]

Pushed:

http://hg.mozilla.org/mozilla-central/rev/6ec3a45f4309