1086684 - (e10s-lazarus) [e10s] "Lazarus: Form Recovery" add-on crashes in PBlobChild::SendGetFilePath() when attaching file to Bugzilla bug

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[martin]

User story:
Open  a new bug in bugzilla or try to attach a file to an existing bug.
Click add an attachment in the bugzilla webpage.
Click "Browse" & Select a file.
Tab will crash shortly after selecting the file and returning to the page

Expected Result:
File Upload

Reproducible: Always
BuildID: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141021030208 CSet: 29fbfc1b31aa

Comment 1

[martin]

Crashreports: 
https://crash-stats.mozilla.com/report/index/bp-3a40c7c9-945a-41dc-9138-4f3fd2141021
https://crash-stats.mozilla.com/report/index/bp-8f2c1586-c34d-4da9-a13b-7bf5f2141021
https://crash-stats.mozilla.com/report/index/bp-a3ca88c1-3cde-4b51-ab29-7d6082141021
https://crash-stats.mozilla.com/report/index/bp-78422bfe-2ee7-404f-8098-ce2192141021
(and several more ;-) )

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey Martin - thanks for reporting! Can you see if you still crash if you have all of your add-ons disabled?

Comment 3

[martin]

Attachment: Firefox Flashplayer Context Menu Bug.pdf

Testing , please ignore.

Comment 4

[martin]

Tested with Addons disabled, crash is gone:
List of previously active Addons:
Addon-Compatibility Reporter 2.0.2
Coookies Manager+ 1.5.2
Download YouTube Videos as MP4 1.7.28
DownThemall 2.0.17
eCleander 1.4
Lazarus:form Recovery 2.3
Nightly Tester Tools 3.7
Saved Password Editor 2.7.3
Secure Password Genarator 0.5
StartupMaster 1.6

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Looks like you were able to attach, so that's good.

Can you disable them one at a time and see if you can narrow in on the one causing the issue?

Comment 6

[martin]

Did retest as requested.
I found the one which is causing the crash:
The culprit is Lazarus:Form Recovery 2.3

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Thanks!

Comment 8

[Chris Peterson [:cpeterson]]

https://addons.mozilla.org/en-US/firefox/addon/lazarus-form-recovery

Comment 9

[martin]

@Chris,
I have retested with e10s disabled, the crash does not happen. 
So the bug is triggered by e10s being enabled.

Comment 10

[martin]

Lazarus 3.0 does not seem to work at all with e10s enabled.

Comment 11

[Chris Peterson [:cpeterson]]

Thanks for testing, Martin. This definitely looks like an e10s bug.

Comment 12

[martin]

I can also trigger this crash (or one that has the same signature like this one)  when selecting the Lazarus menue entry "Recover Form" from the context menu of a text box (Such as the comment textbox in bugzilla)

Comment 13

[martin]

Crashes linked to comment 12 above:
https://crash-stats.mozilla.com/report/index/5e0dfbc4-371f-449a-ac56-2dea72141022
https://crash-stats.mozilla.com/report/index/bp-bc324b45-d89e-4114-bb06-fafc52141022

Comment 14

[martin]

retesting method from comment 12 shows that I can not repro that way on build
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141114030206 CSet: 7f0d92595432

Comment 15

[martin]

Sorry messed up this bug when testing, hope I did undo the unwanted changes (completely).

Comment 16

[martin]

Did repro this bug using the story from comment 1
new crash report for this is at bp bp-9aa69788-bd4c-440a-93ab-b2a802141115
BuildID Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141114030206 CSet: 7f0d92595432

Comment 17

[Francesco Lodolo [:flod]]

Having the same issue here, I'd say the bug is confirmed (should this be in Add-on compatibility?)

Comment 19

[Chris Peterson [:cpeterson]]

Adding "e10s-lazarus" Bugzilla alias because we are getting multiple bug reports related to Lazarus crashes.

Comment 20

[Jorge Villalobos [:jorgev] (he/him)]

The developer has been notified through AMO. However, it looks like the add-on hasn't been updated in over 3 years.

Comment 21

[martin]

@Jorge comment 20 there is a version 3 of the addon at http://getlazarus.com/download where the current development is happening
The Firefox Addons Site still has 2.3 https://addons.mozilla.org/de/firefox/addon/lazarus-form-recovery/

Comment 22

[Jorge Villalobos [:jorgev] (he/him)]

I see. Well, if a compatible version isn't submitted to AMO, we'll downgrade the listing to reduce its visibility.

Comment 23

[martin]

Hi Jorge, I have tried V3 of the addon and since for a time it did not seem to work at ALL with the current nightlies, I chose to stick with V 2.3 .
I'll retry V3 when I find the time to do so.

Comment 25

[[e-mail] (:bhearsum)]

Any update here? I don't want to give up Lazarus, which means I'm not using e10s :(.

Comment 26

[martin]

Just verified that the crash is still here, I guess we would need the addon developer to fix this issue...

Comment 27

[martin]

Still crashes with the same user story on: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150123093302 CSet: a6bbabebed2f

Comment 29

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Blake, would you be able to take this? I think we can just make GetFilePath an urgent message. It appears to be unused except for the file picker in e10s as well as in testing.
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp#4497

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Sure. I'll get to this next week.

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bill McCloskey (:billm) from comment #29)
> Blake, would you be able to take this? I think we can just make GetFilePath
> an urgent message. It appears to be unused except for the file picker in
> e10s as well as in testing.
> http://mxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp#4497

To update the bug here, I looked into this and it turned out to be more complicated than just making the function urgent. The problem is that blobs can be created both in the child and the parent, and parents aren't allowed to send urgent messages to children. Fixing that requires a major rejiggering of the blob code, so I'm going to fix this by stashing the return value at a safe time in the child and avoiding the sync IPC call in HTMLInputElement.value.

Comment 32

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

I'll write a test for this tomorrow.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8575689 [details] [diff] [review]
Patch v1

I couldn't get the test for this to work. I'm going to try again tomorrow, but in the meantime, I don't see any harm in getting review on this now.

Basically, the idea here is to avoid calling GetMozFullPath at random times and to do so only when we know it's safe (and we're not dealing with a CPOW).

Comment 34

[(no longer active)]

Comment on attachment 8575689 [details] [diff] [review]
Patch v1

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

::: dom/html/HTMLInputElement.cpp
@@ +1691,5 @@
>        return NS_OK;
>  
>      case VALUE_MODE_FILENAME:
>        if (nsContentUtils::IsCallerChrome()) {
> +        aValue.Assign(mFirstFilePath);

Please add a comment to describe why calling GetMozFullPath here would be a mistake.

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1.1

I added the requested comment.

Comment 36

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add a mochitest

It's a little wonky that input.value is empty for blob files; nevertheless, I've tested and this mochitest crashes without my patch for the same reason as Lazarus would.

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bdf8097bc20

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8576890 [details] [diff] [review]
Add a mochitest

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

::: dom/ipc/tests/file_bug1086684.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=42976

Wrong bug number.

@@ +3,5 @@
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=42976
> +-->
> +<head>
> +  <title>Test for Bug 42976</title>

Wrong bug number.

@@ +7,5 @@
> +  <title>Test for Bug 42976</title>
> +  <meta charset="UTF-8">
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=42976">Mozilla Bug 42976</a>

Wrong bug number.

@@ +10,5 @@
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=42976">Mozilla Bug 42976</a>
> +<p id="display"></p>
> +<div id="content">
> +    <input type="file" id="f" name="file">

Is the name useful at all?

@@ +12,5 @@
> +<p id="display"></p>
> +<div id="content">
> +    <input type="file" id="f" name="file">
> +</div>
> +<pre id="test">

Needed?

@@ +13,5 @@
> +<div id="content">
> +    <input type="file" id="f" name="file">
> +</div>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">

What's this script tag for?

::: dom/ipc/tests/test_bug1086684.html
@@ +57,5 @@
> +      let blob = new Blob([]);
> +      let file = new File([blob], "helloworld.txt", { type: "text/plain" });
> +
> +      mm.sendAsyncMessage("testBug1086684:parentReady", { file });
> +      lastResult = yield;

Standard mochitests desperately need bug 1078657. :/ add_task would make this so much more linear and readable.

That's not on you - just a gripe.

@@ +59,5 @@
> +
> +      mm.sendAsyncMessage("testBug1086684:parentReady", { file });
> +      lastResult = yield;
> +
> +      // Isn't it weird that we don't get a path at all (even though we have a

Is it weird? Like, weird as in we should file a bug?

Comment 39

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #38)
> Is it weird? Like, weird as in we should file a bug?

I talked to sicking about this and he explained that this is a side-effect of how things are mocked-up. Basically, it's surprising, but simply an odd interaction between DOM files and the (internal) blobs that back them.

Comment 40

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Add a mochitest, v2

I cleaned up the internal file.

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8576942 [details] [diff] [review]
Add a mochitest, v2

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

1 additional nit, but land away. Thanks!

::: dom/ipc/tests/file_bug1086684.html
@@ +8,5 @@
> +  <meta charset="UTF-8">
> +</head>
> +<body>
> +<a target="_blank"
> +    href="https://bugzilla.mozilla.org/show_bug.cgi?id=1086684">Mozilla Bug 1086684</a>

Nit - alignment.

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix b2g mochitests

This patch causes b2g mochitests to crash because we actually assert that we don't use this message at all in b2g (unless we're fuzzing).  With bent out, I didn't want to bother changing that, so this patch just makes the patch not assert. I suppose this makes sense, since chrome code touching content should be really rare in b2g. For more context, see <http://mxr.mozilla.org/mozilla-central/source/dom/ipc/Blob.cpp?rev=f77767481b16#4498>.

Comment 43

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=459c722a0cff

Comment 44

[(no longer active)]

Comment on attachment 8577479 [details] [diff] [review]
Fix b2g mochitests

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

OK, but please check with Ben what the right thing to do here is, when he gets back?  Thanks!

Comment 45

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Really fix b2g

I didn't realize that MOZ_CHILD_PERMISSIONS wasn't automatically defined everywhere. This makes attachment 8577479 [details] [diff] [review] actually work as intended.

Ben signed off on this approach in person.

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8578216 [details] [diff] [review]
Really fix b2g

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

::: dom/html/moz.build
@@ +244,5 @@
>      '/netwerk/base',
>  ]
>  
> +if CONFIG['MOZ_CHILD_PERMISSIONS']:
> +    DEFINES['MOZ_CHILD_PERMISSIONS'] = True

Alternatively, you could add an AC_DEFINE for this variable in configure.in: https://dxr.mozilla.org/mozilla-central/source/configure.in#5716

That would make this define global, rather than directory specific. I don't know the code well enough to know what is best.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Backed out for LSAN leaks.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd02f191756

https://treeherder.mozilla.org/logviewer.html#?job_id=7867650&repo=mozilla-inbound

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Also https://treeherder.mozilla.org/logviewer.html#?job_id=7871959&repo=mozilla-inbound on multiple B2G suites

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #48)
> Also
> https://treeherder.mozilla.org/logviewer.html#?job_id=7871959&repo=mozilla-
> inbound on multiple B2G suites

This is a bug in b2g and is a crash waiting to happen.

Comment 50

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=891c1fefc668

Comment 51

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

I added an assertion when I checked that last patch in and it went orange (as well as going orange for bug 1145854, which my newly-added mochitest exposed). In b2g, a file picker with a picked file was a ticking time bomb waiting for us to blur the page and send the poisoned message to the parent.

I'll hold off on review until I see the results of my most recent try run.

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=306766674c4b

I give up on trying to add any assertions or generally trying to enforce anything related to b2g or not here. The problem that the last try push pointed out was that there are tests that use SpecialPowers to interact with <input type=file>s on b2g, which caused the new assertions to continue failing.

Rather than try to flush out all of the tests that might do this (note: the tests don't assert because the files backing them are same-process blobs and not remote blobs, which is hard to detect from HTMLInputElement) I'm just going to back off, fix this bug for desktop and hope that we don't get too badly burned in the future.

This try run should go green.

Comment 53

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v3

Comment 54

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8582017 [details] [diff] [review]
patch v3

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

</holding-nose>

::: dom/html/HTMLInputElement.cpp
@@ +1691,5 @@
>        return NS_OK;
>  
>      case VALUE_MODE_FILENAME:
>        if (nsContentUtils::IsCallerChrome()) {
> +#ifndef MOZ_CHILD_PERMISSIONS

Nit: reverse these blocks to avoid the double negative

@@ +2677,5 @@
>  
>  void
>  HTMLInputElement::FireChangeEventIfNeeded()
>  {
> +  // NB: It is important to avoid calling GetValue for <input type=file> here.

Nit: Explain why?

Comment 55

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix the remaining oranges

khuey pointed out the massive thinko in my previous patches (MOZ_CHILD_PERMISSIONS was inconsistently defined in various places, and would give HTMLInputElement differing sizes and member offsets depending on what directory a given file was in when it included it). This patch should fix that by defining or not MOZ_CHILD_PERMISSIONS correctly in all directories in the tree.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a778889fd7b8

Comment 56

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6802c3085438
https://hg.mozilla.org/integration/mozilla-inbound/rev/20d97f735cde

Comment 57

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb0b42f9068

My mochitest fails on Android (and then causes test_cpow_cookies.html to fail after it) due to timing out with no input after "prefs are set". I suspect that we can't load over HTTP in child processes on Android, but I'm not set up to test it now.

That being said, the test does not test anything that is currently relevant for Android, so it's not terrible to not run it on that platform.

Comment 58

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6802c3085438
https://hg.mozilla.org/mozilla-central/rev/20d97f735cde
https://hg.mozilla.org/mozilla-central/rev/bcb0b42f9068