Bug 1086684 (e10s-lazarus)

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

RESOLVED FIXED in Firefox 39

Status

()

Core
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: martin, Assigned: mrbkap)

Tracking

({crash, reproducible})

Trunk
mozilla39
x86
Windows 7
crash, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10sm5+, firefox39 fixed)

Details

(crash signature)

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

3 years ago
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
(Reporter)

Comment 1

3 years ago
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 ;-) )

Updated

3 years ago
Blocks: 516752
Hey Martin - thanks for reporting! Can you see if you still crash if you have all of your add-ons disabled?
Flags: needinfo?(martin)
(Reporter)

Comment 3

3 years ago
Created attachment 8509032 [details]
Firefox Flashplayer Context Menu Bug.pdf

Testing , please ignore.
Flags: needinfo?(martin)
(Reporter)

Comment 4

3 years ago
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
Flags: needinfo?(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?
Blocks: 905436
Flags: needinfo?(mconley) → needinfo?(martin)
(Reporter)

Comment 6

3 years ago
Did retest as requested.
I found the one which is causing the crash:
The culprit is Lazarus:Form Recovery 2.3
Flags: needinfo?(martin) → needinfo?(mconley)
Thanks!
Flags: needinfo?(mconley)
Summary: [e10s] Crash when attaching file to bugzilla bug → [e10s] Lazarus: Form Recovery causes crash when attaching file to bugzilla bug
https://addons.mozilla.org/en-US/firefox/addon/lazarus-form-recovery
tracking-e10s: --- → ?
Keywords: crash, reproducible

Updated

3 years ago
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak | mozilla::ipc::MessageChannel::DebugAbort(char const*, int, char const*, char const*, bool) | mozilla::ipc::MessageChannel::Send(IPC::Message*, IPC::Message*) ]
(Reporter)

Comment 9

3 years ago
@Chris,
I have retested with e10s disabled, the crash does not happen. 
So the bug is triggered by e10s being enabled.
Flags: needinfo?(cpeterson)
(Reporter)

Comment 10

3 years ago
Lazarus 3.0 does not seem to work at all with e10s enabled.
Thanks for testing, Martin. This definitely looks like an e10s bug.
Flags: needinfo?(cpeterson)
Summary: [e10s] Lazarus: Form Recovery causes crash when attaching file to bugzilla bug → [e10s] "Lazarus: Form Recovery" add-on crashes in PBlobChild::SendGetFilePath() when attaching file to Bugzilla bug
(Reporter)

Comment 12

3 years ago
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)
(Reporter)

Comment 13

3 years ago
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
tracking-e10s: ? → m5+
(Reporter)

Comment 14

3 years ago
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
No longer blocks: 516752, 905436
Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak | mozilla::ipc::MessageChannel::DebugAbort(char const*, int, char const*, char const*, bool) | mozilla::ipc::MessageChannel::Send(IPC::Message*, IPC::Message*) ]
Keywords: crash, reproducible
Summary: [e10s] "Lazarus: Form Recovery" add-on crashes in PBlobChild::SendGetFilePath() when attaching file to Bugzilla bug → Message Body is uneditable on latest Daily Error in MsgComposeCommands shown in Console
(Reporter)

Comment 15

3 years ago
Sorry messed up this bug when testing, hope I did undo the unwanted changes (completely).
Blocks: 516752, 905436
Crash Signature: Crash Signature: [@ mozalloc_abort(char const* const) | NS_DebugBreak | mozilla::ipc::MessageChannel::DebugAbort(char const*, int, char const*, char const*, bool) | mozilla::ipc::MessageChannel::Send(IPC::Message*, IPC::Message*) ]
Keywords: crash, reproducible
Summary: Message Body is uneditable on latest Daily Error in MsgComposeCommands shown in Console → [e10s] "Lazarus: Form Recovery" add-on crashes in PBlobChild::SendGetFilePath() when attaching file to Bugzilla bug
(Reporter)

Comment 16

3 years ago
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
Having the same issue here, I'd say the bug is confirmed (should this be in Add-on compatibility?)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 1096496
Adding "e10s-lazarus" Bugzilla alias because we are getting multiple bug reports related to Lazarus crashes.
Alias: e10s-lazarus
The developer has been notified through AMO. However, it looks like the add-on hasn't been updated in over 3 years.
(Reporter)

Comment 21

3 years ago
@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/
I see. Well, if a compatible version isn't submitted to AMO, we'll downgrade the listing to reduce its visibility.
(Reporter)

Comment 23

3 years ago
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.
Duplicate of this bug: 1111315
Any update here? I don't want to give up Lazarus, which means I'm not using e10s :(.
Assignee: nobody → wmccloskey
(Reporter)

Comment 26

3 years ago
Just verified that the crash is still here, I guess we would need the addon developer to fix this issue...
(Reporter)

Comment 27

3 years ago
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
Duplicate of this bug: 1096644
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
Flags: needinfo?(mrbkap)
(Assignee)

Comment 30

3 years ago
Sure. I'll get to this next week.
Assignee: wmccloskey → mrbkap
Flags: needinfo?(mrbkap)
(Assignee)

Comment 31

3 years ago
(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.
(Assignee)

Comment 32

3 years ago
Created attachment 8575689 [details] [diff] [review]
Patch v1

I'll write a test for this tomorrow.
(Assignee)

Comment 33

3 years ago
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).
Attachment #8575689 - Flags: review?(ehsan)

Comment 34

3 years ago
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.
Attachment #8575689 - Flags: review?(ehsan) → review+
(Assignee)

Comment 35

3 years ago
Created attachment 8576887 [details] [diff] [review]
Patch v1.1

I added the requested comment.
(Assignee)

Comment 36

3 years ago
Created attachment 8576890 [details] [diff] [review]
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.
Attachment #8575689 - Attachment is obsolete: true
(Assignee)

Comment 37

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bdf8097bc20
(Assignee)

Updated

3 years ago
Attachment #8576890 - Flags: review?(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?
Attachment #8576890 - Flags: review?(mconley) → review-
(Assignee)

Comment 39

3 years ago
(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.
(Assignee)

Comment 40

3 years ago
Created attachment 8576942 [details] [diff] [review]
Add a mochitest, v2

I cleaned up the internal file.
Attachment #8576890 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8576942 - Flags: review?(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.
Attachment #8576942 - Flags: review?(mconley) → review+
(Assignee)

Comment 42

3 years ago
Created attachment 8577479 [details] [diff] [review]
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>.
(Assignee)

Comment 43

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=459c722a0cff
(Assignee)

Updated

3 years ago
Attachment #8577479 - Flags: review?(ehsan)

Comment 44

3 years ago
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!
Attachment #8577479 - Flags: review?(ehsan) → review+
Component: General → General
Product: Firefox → Core
(Assignee)

Comment 45

3 years ago
Created attachment 8578216 [details] [diff] [review]
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.
Attachment #8578216 - Flags: review?(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.
Attachment #8578216 - Flags: review?(gps) → review+
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
Also https://treeherder.mozilla.org/logviewer.html#?job_id=7871959&repo=mozilla-inbound on multiple B2G suites
(Assignee)

Comment 49

3 years ago
(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.
(Assignee)

Updated

3 years ago
Depends on: 1145854
(Assignee)

Comment 50

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=891c1fefc668
(Assignee)

Comment 51

3 years ago
Created attachment 8580954 [details] [diff] [review]
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.
Attachment #8576887 - Attachment is obsolete: true
Attachment #8577479 - Attachment is obsolete: true
Attachment #8578216 - Attachment is obsolete: true
(Assignee)

Comment 52

3 years ago
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.
(Assignee)

Comment 53

3 years ago
Created attachment 8582017 [details] [diff] [review]
patch v3
Attachment #8576942 - Attachment is obsolete: true
Attachment #8580954 - Attachment is obsolete: true
Attachment #8582017 - Flags: review?(bent.mozilla)
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?
Attachment #8582017 - Flags: review?(bent.mozilla) → review+
(Assignee)

Updated

3 years ago
Blocks: 1147188
(Assignee)

Comment 55

3 years ago
Created attachment 8582748 [details] [diff] [review]
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
(Assignee)

Updated

3 years ago
Attachment #8582748 - Flags: review?(gps)

Updated

3 years ago
Attachment #8582748 - Flags: review?(gps) → review+
(Assignee)

Comment 56

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6802c3085438
https://hg.mozilla.org/integration/mozilla-inbound/rev/20d97f735cde
(Assignee)

Comment 57

3 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/6802c3085438
https://hg.mozilla.org/mozilla-central/rev/20d97f735cde
https://hg.mozilla.org/mozilla-central/rev/bcb0b42f9068
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.