1316389 - Port bug 1303196 to TB - CRASH (MOZ_RELEASE_ASSERT) when click particular link on particular WEB-page

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1316104 +++

Opening a webpage in TB is not so easy.

To prepare for this bug, install the WebApp Tabs add-on:
https://addons.mozilla.org/en-US/thunderbird/addon/webapp-tabs/

Save a draft message that contains this link:
http://www.notebookcheck.com/Google-Suche.36689.0.html?cx=partner-pub-9323363027260837%3Atg143v-nekt&cof=FORID%3A10&ie=UTF-8&q=i3-5005U&sa=

Configure WebApp Tabs so http://www.notebookcheck.com is a web app and it's pages will load in TB.

Click in the link in the draft to open the page in TB.

Search for "Der Intel Core i3-5005U ist ein sparsamer" in the page and click on the "Intel inside" icon to the left.

Crash!

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1316389.patch [landed in comment #10 and #11]

Hmm, ported attachment 8809081 [details] [diff] [review] from bug 1316104 and it still crashes.

This could also be caused by the add-on.

Alta88, could you please take a look. With one of your add-ons you can also open pages in TB.

Comment 2

[alta88]

it works fine for me with my addon.  there's nothing in Tb that uses .browserDOMWindow = new nsBrowserAccess() so that's some very old artifact that precedes the right way to open web pages in tabs.  as for that extension, not really interested in whether it's doing the right thing, why are you?  

i'd get rid of the browserDOMWindow init, tell extensions to use contentTab methods (webNavigation), and mark this wfm..

Comment 3

[Jorg K (CEST = GMT+2)]

Thanks for looking into it, let me clarify:

First of all, bug 1303196 is real. The interface on openURI: function () has changed. OK, I added some debug, looks like this isn't run at all.

Did you follow the steps and TB didn't crash for you? It crashed for me. You're saying it's purely due to the add-on? I'm not interested in the add-on but I need to make sure that TB doesn't crash.

What would you like to get rid of? function nsBrowserAccess()? Would you be able to submit a patch?

Also, can you please refresh my memory about "your add-on". What was it called again and what would be the STR using it?

Comment 4

[alta88]

I didn't, and am not going to, install that addon and test it. So if it's the addon crashing it, well I'm sure any addon can crash Tb in numerous ways in xpcom.

It turns out browserDOMWindow does need to be initialized with nsBrowserAccess; go to an addons tab and Get Addons page and click a link to exercise it and open the link in a new tab. It works fine on yesterday's daily.  

My addon is https://totalmessage.mozdev.org/ and it adds context menus to open any link in a new tab, but using tabmail.openTab and not browserDOMWindow.openURI.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8809121 [details] [diff] [review]
1316389.patch [landed in comment #10 and #11]

This is what SM landed:
https://hg.mozilla.org/comm-central/rev/0fa8bfa73cdec9511df0395c2b1f3af2e5fa2b83

I don't know how to test it.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 8809121 [details] [diff] [review]
1316389.patch [landed in comment #10 and #11]

Review of attachment 8809121 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable. r=mkmelin

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 8809121 [details] [diff] [review]
1316389.patch [landed in comment #10 and #11]

Frank-Rainer, I've done what you've done but I have no way to test this. Can you help?

Comment 9

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8809121 [details] [diff] [review]
1316389.patch [landed in comment #10 and #11]

I wouldn't really call me an expert on this but as alta88 stated openURI seems not to be used in TB code so I would just put the patch in an wait for eventual fallout. I think only an add-on which opens external urls might use this.

Comment 10

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/1d2132213242cb7967889420a3d1ff2e490c3e71
(I landed it myself after all since I needed to change the reviewer in the commit message.)

Comment 11

[Jorg K (CEST = GMT+2)]

Ratty pointed out the clumsiness in that patch (bug 1316104 comment #24), so I fixed it:
https://hg.mozilla.org/comm-central/rev/9245026f182fb3f20c12698d7197b3938c88b6f8

Comment 12

[Jorg K (CEST = GMT+2)]

Looks like the fix wasn't complete as further development in bug 1316104 has shown.

FRG was kind enough to supply a patch, but I still don't know how to test this.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: Follow-up patch: 1316389-untested-tb.patch [not required, see comment #36]

I'm really struggling with this. You guys know better than me.
This is was SM landed:
https://hg.mozilla.org/comm-central/rev/4df935f993b2e7dac566661b712ba4d915b0c1d0

Comment 14

[alta88]

Comment on attachment 8813433 [details] [diff] [review]
Follow-up patch: 1316389-untested-tb.patch [not required, see comment #36]

1. There are no documented crashes in Tb (Bug 1316104 comment 45).
2. You can't reproduce (it should be a simple crafted call to openURI in the js console).

Opening a Tb tab is fundamentally different from Fx/Sm and this is reflected in the custom code in nsBrowserAccess. I don't know what you're chasing then, unless it's just a copy/paste.

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to alta88 from comment #14)
> 2. You can't reproduce (it should be a simple crafted call to openURI in the
> js console).
Can you do this for me, please. Please document it here, so I can add a comment to the patch so that the next guy along can reproduce it.

> I don't know what you're chasing
> then, unless it's just a copy/paste.
We have some code in the box, which is potentially broken. If it's unused, let's remove it, that's what you suggested in comment #2 but took back in comment #4. If it's broken let's fix it. Having it broken and potentially crashing doesn't appear to be an option to me. And yes, it's a copy/paste from SM, but we still need to make sure it fixes the potential problem.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: openUri-test.patch

I'm struggling with this bug. Aceman, can you help. The first patch got landed in comment #10 and comment #11. Now I want to decide on whether to land the follow-up patch.

To test this, I added a call to openURI() to the function that changes the charset of a message: View > Text Encoding.

I have two test cases. Neither of them actually opens the URL I asked for, but they also don't crash. With the follow-up patch the behaviour doesn't change.

Alta88 thinks that this is a useless discussion since this function doesn't appear to be used. We're merely following:
https://dxr.mozilla.org/comm-central/rev/bad312aefb42982f492ad2cf36f4c6c3d698f4f7/mozilla/browser/base/content/browser.js#4925

Comment 17

[Magnus Melin [:mkmelin]]

Comment on attachment 8813433 [details] [diff] [review]
Follow-up patch: 1316389-untested-tb.patch [not required, see comment #36]

Review of attachment 8813433 [details] [diff] [review]:
-----------------------------------------------------------------

I don't really have an opinion on this, but if there are no known problems maybe not worth doing anything about them.

Comment 18

[:aceman]

Is this crashing in c-c code or m-c code? Do you have a stack trace for the crash?

Comment 19

[Jorg K (CEST = GMT+2)]

I have nothing, it's basically a port of bug 1316104 which is crashing in SM.

Comment 20

[Jorg K (CEST = GMT+2)]

No crashes in TB, see comment #14.

Comment 21

[:aceman]

I read comment 0 so that you get a crash. So you don't. And this bug is about wondering why TB does not crash and if we need to port anything?

Comment 22

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #21)
> I read comment 0 so that you get a crash. So you don't.
I get the crash when using the WebApp Tabs add-on. Without it, I can't open the URL in question in a tab. So instead I wrote a patch to misuse another function to trigger opening a URL.

> And this bug is
> about wondering why TB does not crash and if we need to port anything?
Right. Also, why doesn't the URL open with my patch? If it opened, we could try with the URL from comment #0.

Comment 23

[:aceman]

(In reply to Jorg K (GMT+1) from comment #22)
> (In reply to :aceman from comment #21)
> > I read comment 0 so that you get a crash. So you don't.
> I get the crash when using the WebApp Tabs add-on. Without it, I can't open
> the URL in question in a tab. So instead I wrote a patch to misuse another
> function to trigger opening a URL.

But then you are crashing.

You landed 2 patches in TB. Now you have the third patch that is mimicking Seamonkey's third patch.

Do you get the crash with or without that third patch with the addon? I'm not sure what is not working here. Is the third patch not working for TB while it is working for SM?

Comment 24

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #23)
> But then you are crashing.
No, the add-on is crashing. As far as I can see, the add-on does not run openURI().

> You landed 2 patches in TB. Now you have the third patch that is mimicking
> Seamonkey's third patch.
Right. Let's call that 3rd patch.

> Do you get the crash with or without that third patch with the addon?
I didn't try since the add-on is not executing that code.

> I'm not sure what is not working here. Is the third patch not working for TB
> while it is working for SM?
Well, the openUri-test.patch patch (let's refer to it as "test patch") is not working since it doesn't open the URL I requested. As I said in comment #16:

I have two test cases. Neither of them actually opens the URL I asked for, but they also don't crash. With the follow-up patch (3rd patch) the behaviour doesn't change.

Aceman, if you want to help (please!) then you need to play around with it and see whether you can figure something out.

Comment 25

[Frank-Rainer Grahl (:frg)]

Personally I am seeing nothing in TB which uses it. If you don't find an extension which needs it the code could probably be removed.

Unlike FF and SM mail/components/nsMailDefaultHandler.js uses a different method to open external content so not here either.

Comment 26

[Jorg K (CEST = GMT+2)]

FRG, look at comment #4. I will play around with that a little:

(In reply to alta88 from comment #4)
> It turns out browserDOMWindow does need to be initialized with
> nsBrowserAccess; go to an addons tab and Get Addons page and click a link to
> exercise it and open the link in a new tab. It works fine on yesterday's
> daily.

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: Crash dump

This may be unrelated, but I followed the steps from comment #4 in a local debug build. On the "Get Add-ons" page I clicked on "Mail Merge" and it crashed on a MOZ_ASSERT.

This may be unrelated since the debug I put into openURI() didn't show up.

Comment 28

[Frank-Rainer Grahl (:frg)]

Sure for nsBrowserAccess but I don't see openURI used anywhere. But still not firm with all the stuff after more than a year.

Comment 29

[Jorg K (CEST = GMT+2)]

Hmm, seems unrelated. It crashes even with the follow-up patch (attachment 8813433 [details] [diff] [review]) and even if I remove openURI().

Alta88, do you think we can remove openURI() altogether? nsBrowserAccess() can't be removed, well, at least I get JS errors if I do since it's referenced in a few spots.

Comment 30

[alta88]

i'm not going to look at this any further.

Comment 31

[Jorg K (CEST = GMT+2)]

To be continued in bug 1320954 and bug 1320958.

Thanks, FRG for your patch (attachment 8813433 [details] [diff] [review]), but since we can't confirm whether there is a crash or not and whether the function is even used I won't apply it. Instead I'm proposing to remove openURI() in bug 1320954.

Comment 32

[Jorg K (CEST = GMT+2)]

OK, I'm back. Bug 1320958 is invalid since an add-on (WebApp Tabs) caused the crash. Bug 1320954 is invalid, since openURI() *is* used, see bug 1320954 comment #2. So now that I now that it's used I can try to make it crash. Let's see.

Comment 33

[Jorg K (CEST = GMT+2)]

Looking into this further. If 'aOpener' is null, 'referrer' is undefined, so an exception is thrown and I end up in the catch. So this doesn't work as intended.

Comment 34

[Jorg K (CEST = GMT+2)]

Oh, I forgot to paste:
https://dxr.mozilla.org/comm-central/rev/4f8486123b817d6b18bf2684a0f7e5facf725ce6/mail/base/content/mailWindow.js#693

Comment 35

[Jorg K (CEST = GMT+2)]

Comment on attachment 8815216 [details]
Crash dump

(Crash was caused by WebApp Tabs add-on.)

Comment 36

[Jorg K (CEST = GMT+2)]

My testing is bug 1321013 has shown that the additional patch in attachment 8813433 [details] [diff] [review] is not required.

If you load a URL via openURI() (which is repaired in bug 1321013) any link on the page loaded will open in the browser and not in TB itself.

Apparently the crash in SM happened when a link with target="_blank" is opened in a new tab. Since TB opens all links in the browser, we don't have a problem here.

So I think we can finally put this bug to rest ;-)

Comment 37

[Jorg K (CEST = GMT+2)]

Comment on attachment 8814546 [details] [diff] [review]
openUri-test.patch

Moved to bug 1321013.