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

RESOLVED FIXED in Thunderbird 52.0

Status

Thunderbird
General
--
major
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: Jorg K (GMT+2), Unassigned)

Tracking

({crash})

Trunk
Thunderbird 52.0
Unspecified
Windows 7
crash
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [easyconfirm], crash signature, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 months ago
+++ 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!
(Reporter)

Updated

8 months ago
Summary: CRASH (MOZ_RELEASE_ASSERT) when click particular link on particular WEB-page → Port bug 1303196 to TB - CRASH (MOZ_RELEASE_ASSERT) when click particular link on particular WEB-page
(Reporter)

Comment 1

8 months ago
Created attachment 8809121 [details] [diff] [review]
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.
Flags: needinfo?(alta88)

Comment 2

8 months ago
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..
Flags: needinfo?(alta88)
(Reporter)

Comment 3

8 months ago
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?
Flags: needinfo?(alta88)

Comment 4

8 months ago
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.
Flags: needinfo?(alta88)
(Reporter)

Comment 5

8 months ago
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.
Attachment #8809121 - Flags: review?(mkmelin+mozilla)
Attachment #8809121 - Flags: review?(acelists)

Updated

8 months ago
Depends on: 1316561

Comment 6

8 months ago
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
Attachment #8809121 - Flags: review?(mkmelin+mozilla)
Attachment #8809121 - Flags: review?(acelists)
Attachment #8809121 - Flags: review+
(Reporter)

Comment 7

8 months ago
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?
Attachment #8809121 - Flags: feedback?(frgrahl)

Updated

8 months ago
Duplicate of this bug: 1316561
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.
Attachment #8809121 - Flags: feedback?(frgrahl) → feedback+
(Reporter)

Updated

8 months ago
Keywords: checkin-needed
(Reporter)

Comment 10

8 months ago
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.)
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
(Reporter)

Comment 11

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

Comment 12

7 months ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 13

7 months ago
Created attachment 8813433 [details] [diff] [review]
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
Attachment #8813433 - Flags: feedback?(mkmelin+mozilla)
Attachment #8813433 - Flags: feedback?(alta88)

Comment 14

7 months ago
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.
Attachment #8813433 - Flags: feedback?(alta88)
(Reporter)

Comment 15

7 months ago
(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.
Flags: needinfo?(alta88)

Updated

7 months ago
Flags: needinfo?(alta88)
(Reporter)

Comment 16

7 months ago
Created attachment 8814546 [details] [diff] [review]
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
Flags: needinfo?(acelists)

Comment 17

7 months ago
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.
Attachment #8813433 - Flags: feedback?(mkmelin+mozilla)

Comment 18

7 months ago
Is this crashing in c-c code or m-c code? Do you have a stack trace for the crash?
(Reporter)

Comment 19

7 months ago
I have nothing, it's basically a port of bug 1316104 which is crashing in SM.
(Reporter)

Comment 20

7 months ago
No crashes in TB, see comment #14.

Comment 21

7 months ago
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?
(Reporter)

Comment 22

7 months ago
(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

7 months ago
(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?
Flags: needinfo?(acelists)
(Reporter)

Comment 24

7 months ago
(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.
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.
(Reporter)

Comment 26

7 months ago
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.
(Reporter)

Comment 27

7 months ago
Created attachment 8815216 [details]
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.
Sure for nsBrowserAccess but I don't see openURI used anywhere. But still not firm with all the stuff after more than a year.
(Reporter)

Comment 29

7 months ago
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.
(Reporter)

Updated

7 months ago
Flags: needinfo?(alta88)

Comment 30

7 months ago
i'm not going to look at this any further.
Flags: needinfo?(alta88)
(Reporter)

Comment 31

7 months ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 8 months ago7 months ago
Resolution: --- → FIXED
(Reporter)

Comment 32

7 months ago
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.
(Reporter)

Comment 33

7 months ago
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.
(Reporter)

Comment 34

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

Comment 35

7 months ago
Comment on attachment 8815216 [details]
Crash dump

(Crash was caused by WebApp Tabs add-on.)
Attachment #8815216 - Attachment is obsolete: true
(Reporter)

Comment 36

7 months ago
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 ;-)
(Reporter)

Comment 37

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

Moved to bug 1321013.
Attachment #8814546 - Attachment is obsolete: true
(Reporter)

Updated

7 months ago
Attachment #8809121 - Attachment description: 1316389.patch → 1316389.patch [landed in comment #10 and #11]
(Reporter)

Updated

7 months ago
Attachment #8813433 - Attachment description: Follow-up patch: 1316389-untested-tb.patch → Follow-up patch: 1316389-untested-tb.patch [not required, see comment #36]
You need to log in before you can comment on or make changes to this bug.